Security: Investigate buffer boundary warning in src/signalhandler.cpp

[hoffie]

Describe the bug Codacy emits a warning for src/signalhandler.cpp line 85.

From @ann0see in https://github.com/jamulussoftware/jamulus/issues/314#issuecomment-761890259

Note: I don't see a buffer around that line and was wondering whether the reference could be outdated or wrong. However, this file has not seen any recent updates, as far as I can see.

To Reproduce Run a Codacy scan

Expected behavior

No warning.

Screenshots

"Check buffer boundaries if used in a loop including recursive loops (CWE-120, CWE-20)."

in src/signalhandler.cpp line 85

Version of Jamulus Assuming that @ann0see had master checked, this would imply that the version was 80e1d2325a43aae00667849a41e6cbbdc4b4f163 or before.

@softins Assigning to you as you sounded like you were to look into this? :)

[softins]

OK, it looks to be referring to the ::read() in this function:

void CSignalHandler::OnSocketNotify( int socket )
{
    int sigNum;
    if ( ::read ( socket, &sigNum, sizeof ( int ) ) == sizeof ( int ) )
    {
        emitSignal ( sigNum );
    }
}

It is using the int sigNum as a buffer, and it should be ok because the read is limited to sizeof(int) bytes in size.

Maybe Codacy got confused because the read was not into a char buffer?

@hoffie, where is this Codacy? Is this something you ran on your system, or that Github ran in the CI?

EDIT: OK, just followed the link to @ann0see's comment. I don't think this particular line is a problem.

[pljones]

I think it's a false positive.

[hoffie]

@hoffie, where is this Codacy? Is this something you ran on your system, or that Github ran in the CI?

Not sure, @ann0see did this. However, I just found that Codacy seems free for Open Source projects and I'm just trying the Github app out in my personal Github account on my Jamulus fork. Will report back what it finds. We could discuss whether it makes sense to integrate it into the jamulussoftware org. I'll follow up on this later. I'm a bit wary because it requires extensive (write) permissions.

[hoffie]

Codacy now lists 5 security-related things to check. The first is the one which @ann0see mentioned. I'm providing a screenshot of the detailed view below, but besides confirming that we've looked at the right file, it does not any useful details:

To me, it sounds like a generic hint to be careful about buffer sizes. However, the concrete line seems correct as @softins detailed. In addition, the source of the input is a socket which is used for catching unix signals. In other words, there is no crossing of trust boundaries. So, I neither think that it is exploitable nor that it would be relevant relevant even if it was. Therefore, no security issue.

The other four findings are related to the third-party nsProcess.dll build. They only look like warnings ("don't use lstrcpyn or be careful"), too. Also, as far as I understand, nsProcess.dll is used as part of the Windows installer, so it would be working on trusted input only. No crossing of trust boundaries either.

Personally I don't plan to follow up on adding Codacy to the official organization for now (mainly due to the required permissions). If someone wants to, I'd suggest opening a new issue for that.

I would therefore suggest closing this issue, but will leave it to @softins or @pljones.

[softins]

@hoffie thanks for looking at this! I agree with your assessment.

The only thing that I would find interesting is in the first one, whether Codacy gives the same warning if &sigNum is cast to char *. Either the C-style (char *)&sigNum or the C++-style reinterpret_cast<char*>(&sigNum). They both do the same, and in this context I prefer the C-style cast for conciseness.

[softins]

In any case, we could add a comment to the effect of: // Codacy gives a warning about this line, but it is fine.

[hoffie]

The only thing that I would find interesting is in the first one, whether Codacy gives the same warning if &sigNum is cast to char . Either the C-style (char )&sigNum or the C++-style reinterpret_cast<char*>(&sigNum). They both do the same, and in this context I prefer the C-style cast for conciseness.

Have tried that, doesn't seem to make a difference. See here: https://github.com/hoffie/jamulus/pull/2

Codacy seems to use flawfinder in this check.

Seems like others have hit the same issue and were confused: https://stackoverflow.com/questions/59293533/a-flaw-reported-by-flawfinder-but-i-dont-think-it-makes-sense

There is a way to exclude certain lines from flawfinder, by using a special comment: // Flawfinder: ignore

@softins I created a PR to add an explaining comment as you suggested + the special ignore syntax: #1084.

[pljones]

They both do the same, and in this context I prefer the C-style cast for conciseness.

Volker was eliminating them and moving to full C++ standards-based casts. Apparently there are benefits.

[npostavs]

I wonder if using sizeof sigNum instead of sizeof ( int ) would help the Flawfinder thing see that the buffer size is okay. IMO it's clearer for a human reader that way as well, since there is no need to check the type of sigNum.

[atsampson]

No, flawfinder is a very simple pattern-matching tool, and all it cares about there is that the read function is being used - it doesn't parse the arguments or do any semantic analysis. It's not really intended for use without a human looking over its shoulder.