Fix potential security vulnerability

[GraysonNocera]

Hello, I was running some CodeQL queries on this project when I came across a potential unbounded write that could cause buffer overflow. I have implemented the fix in this PR.

[evelikov]

Please squash the commit and add appropriate commit message. If possible avoid adding pictures in PRs but instead describe in your own words the issue at hand and how the commit addresses it.

Personally I am not worried about any security implications, since to trigger a specially crafted input file which is available in root-only location.

[stoeckmann]

Adding sprintf and a variable instead of fixed format string rises more red flags than an sscanf for all these static code scanners. ;)

[stoeckmann]

The usage of sscanf(... "%s") is not a general security issue. The input string must be long enough to overflow the target buffer. This means that buf must be larger than modname or devname.

Realistically, all Linux systems these days have a PATH_MAX of 4096. To make this code robust, it would be easiest to declare buf with PATH_MAX instead of 4096. That would be an adjustment of exactly one line of code and no binary change.

Proof of Concept about sscanf safety:

#include <stdio.h>
#include <string.h>

int
main(int argc, char *argv[])
{
        char buf[4096], buf2[4096];
        memset(buf, 'A', sizeof(buf));
        buf[sizeof(buf) - 1] = '\0';
        sscanf(buf, "%s", &buf2);
        return 0;
}

So I would prefer if we apply this patch instead:

diff --git a/tools/static-nodes.c b/tools/static-nodes.c
index cb8afd3..d7557f4 100644
--- a/tools/static-nodes.c
+++ b/tools/static-nodes.c
@@ -144,7 +144,7 @@ static void help(void)
 static int do_static_nodes(int argc, char *argv[])
 {
 	struct utsname kernel;
-	char modules[PATH_MAX], buf[4096];
+	char modules[PATH_MAX], buf[PATH_MAX];
 	const char *output = "/dev/stdout";
 	FILE *in = NULL, *out = NULL;
 	const struct static_nodes_format *format = &static_nodes_format_human;

[stoeckmann]

If this commit is about protecting us against a PATH_MAX which is not 4096, then this sprintf call is actually dangerous. You could overflow the format buffer if PATH_MAX is sufficiently large.

Yet it's also just theoretical. A system with such a huge PATH_MAX would probably not compile the code at all because the compiler can already figure out that the stack wouldn't be large enough for all these variables which are declared like char buf[PATH_MAX] etc.

So let's go with the simple diff I've recommended. Avoids introduction of yet another sprintf call.

[GraysonNocera]

Hello, thank you for the replies @stoeckmann and @evelikov. I fully understand and agree with the concerns about the new sprintf. I have squashed the commits and implemented the one-line fix.

[evelikov]

@GraysonNocera cannot see a commit message which explains the problem and/or solution. Check through git log for examples.

[GraysonNocera]

@GraysonNocera cannot see a commit message which explains the problem and/or solution. Check through git log for examples.

Added a more descriptive commit message

[evelikov]

Thanks, updated commit looks good IMHO.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tools/static-nodes.c	0.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
tools/static-nodes.c	0.00% <0.00%> (ø)

[lucasdemarchi]

@GraysonNocera a few things:

• It's not appropriate to add a Signed-off-by from someone else. The first Signed-off-by should match the author of the patch. So I think what you want is:

Signed-off-by: Grayson Nocera <[email protected]>
Suggested-by: Tobias Stoeckmann <[email protected]>

• "This fix adresses the issue described in the pull request." is unneeded redirection. : the commit message should describe the issue rather than point to the pull request. It looks like the rest of the commit message already describes it so you can drop the first part.

[GraysonNocera]

@GraysonNocera a few things:

• It's not appropriate to add a Signed-off-by from someone else. The first Signed-off-by should match the author of the patch. So I think what you want is:

Signed-off-by: Grayson Nocera <[email protected]>
Suggested-by: Tobias Stoeckmann <[email protected]>

• "This fix adresses the issue described in the pull request." is unneeded redirection. : the commit message should describe the issue rather than point to the pull request. It looks like the rest of the commit message already describes it so you can drop the first part.

Updated commit message @lucasdemarchi

[lucasdemarchi]

Applied, thanks.