Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jack_lsp: An attempt to use --server/-s argument fails with buffer overflow #88

Open
unclechu opened this issue Jun 13, 2024 · 4 comments · May be fixed by #89
Open

jack_lsp: An attempt to use --server/-s argument fails with buffer overflow #88

unclechu opened this issue Jun 13, 2024 · 4 comments · May be fixed by #89

Comments

@unclechu
Copy link

$ jack_lsp -s default
*** buffer overflow detected ***: terminated
Aborted (core dumped)

Version 4 (GIt tag) of this repo.

@unclechu
Copy link
Author

I think I figured it out, there’s no string termination in the copied optarg. Here is some debug code I used to test it:

			for (int i=0; i<strlen(optarg); ++i)
				printf("str %d: “%c”\n", i, optarg[i]);
			printf("done\n");
str 0: “d”
str 1: “e”
str 2: “f”
str 3: “a”
str 4: “u”
str 5: “l”
str 6: “t”
done

So, there are only chars from the argument in there, no string termination char. Here is a patch that fixes the bug:

diff --git a/tools/lsp.c b/tools/lsp.c
index 50868b3..f043ad2 100644
--- a/tools/lsp.c
+++ b/tools/lsp.c
@@ -122,7 +122,8 @@ main (int argc, char *argv[])
 	while ((c = getopt_long (argc, argv, "s:AclLphvtuU", long_options, &option_index)) >= 0) {
 		switch (c) {
 		case 's':
-			server_name = (char *) malloc (sizeof (char) * strlen(optarg));
+			server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
+			server_name[strlen(optarg)] = '\0';
 			strcpy (server_name, optarg);
 			options |= JackServerName;
 			break;

unclechu added a commit to unclechu/jack-example-tools that referenced this issue Jun 13, 2024
In the copied `optarg` there are only chars from the argument, no string
termination `\0` char. That causes the following code to go beyond the
variable memory bound.

This change adds one extra char allocation for the `server_name`
variable and puts `\0` string termination char to the last char.

Fixes jackaudio#88
@unclechu unclechu linked a pull request Jun 13, 2024 that will close this issue
@unclechu
Copy link
Author

Created a merge request with the fix: #89

@unclechu
Copy link
Author

In case any Nix folk in need stumble upon this before it is fixed here is what you can use as a temporary solution:

(pkgs.jack-example-tools.overrideAttrs (old: {
  patches = (old.patches or []) ++ [
    # Big fix for “jack_lsp” “--server” argument buffer overflow.
    # Track the bug fixing progress here:
    # https://github.com/jackaudio/jack-example-tools/issues/88
    (pkgs.fetchpatch {
      name = "jack_lsp-fix-jack-server-argument-buffer-overflow.patch";
      url = "https://github.com/jackaudio/jack-example-tools/pull/89/commits/62aeea4c432c8f91b14888c4dc4c310ef762a865.patch";
      hash = "sha256-TbgJdwsxo9K6wTQ46yHLYDbIJkINNARlb332qC8TWlM=";
    })
  ];
}))

unclechu added a commit to unclechu/nixos-config that referenced this issue Jun 13, 2024
@unclechu
Copy link
Author

I grepped other argument parsing pieces for other apps from the repo:

$ git grep optarg | rg malloc | rg -v lsp
example-clients/metro.c:                        bpm_string = (char *) malloc ((strlen (optarg) + 5) * sizeof (char));
example-clients/metro.c:                        client_name = (char *) malloc ((strlen (optarg) + 1) * sizeof (char));
tools/connect.c:                        server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
tools/netsource.c:                peer_ip = (char *) malloc (sizeof (char) * strlen (optarg) + 1);
tools/netsource.c:                client_name = (char *) malloc (sizeof (char) * strlen (optarg) + 1);
tools/netsource.c:                server_name = (char *) malloc (sizeof (char) * strlen (optarg) + 1);
tools/wait.c:                   server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
tools/wait.c:                   client_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));

And all of them except lsp.c seem to add one extra char to the total count. Though for instance in wait.c the string termination char is not set explicitly:

		case 's':
			server_name = (char *) malloc (sizeof (char) * (strlen(optarg) + 1));
			strcpy (server_name, optarg);
			options |= JackServerName;
			break;

Is this okay? Maybe I’m wrong about optarg not having the termination char, it could be just that strlen(optarg) does not take that termination char into account, which is how it is supposed to be, duh. So the second server_name[strlen(optarg)] = '\0'; line in the fix is probably a redundant one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant