C++: Improve cpp/system-data-exposure #7933
Conversation
cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/tests2.cpp
Outdated
Show resolved
Hide resolved
| @@ -67,6 +67,6 @@ void CWE535_Info_Exposure_Shell_Error__w32_char_01_bad() | |||
| printLine("Unable to login."); | |||
| } | |||
| /* FLAW: Write sensitive data to stderr */ | |||
| fprintf(stderr, "User attempted access with password: %s\n", password); | |||
| fprintf(stderr, "User attempted access with password: %s\n", password); // [NOT DETECTED] | |||
There was a problem hiding this comment.
This is unfortunate, but I'm a bit surprised we don't treat fprintf arguments as a sink. It might be worth a separate query for specifically the password-to-console case.
There was a problem hiding this comment.
Yes, I see what you're saying. Flagging writes to stdout / stderr is very noisy but we could write a special case for the very most sensitive information (literally just passwords) being output in such a way. It would catch the SAMATE results but might or might not find much in the real world. I'll try a prototype...
There was a problem hiding this comment.
Quick prototype: https://lgtm.com/query/7564791297006366678/
Results are thin but do exist. I agree this should be a separate query.
|
This PR should be ready to merge. There will be some follow-up work:
|
Is there a DCA run somewhere? I'd be happy to merge it if that looks good! |
|
DCA results: it appears this query |
|
New DCA results (now including this query!): performance of the new query is about average and overall analysis time is barely impacted, though there is some non-tiny wobble in some individual projects. No changes to results. |
|
It looks like we need an internal PR to sync the test changes, right? |
|
Ah, I hadn't noticed that! I'll put one together in about an hour... |
geoffw0
added
the
depends on internal PR
|
Internal PR made, ready to merge (together). |
Improve
cpp/system-data-exposure(CWE-497):getenvwe now try to guess whether the particular environment variable is sensitivesysconfis excluded because all the data it can provide seems fairly benignRemoteFlowSinkFunctionfor sinksstdoutorstderr, thus the SAMATE test is no longer passed - but I don't think a reasonable more accurate version of this query can ever pass that test case.Results are much more accurate (and much fewer). Its an open question whether they're good enough to warrant
@precision high. I'm not sure I can go much further on precision. I could probably restore a few of the flows we've lost (i.e. find more results) though.LGTM run:
https://lgtm.com/query/8145818204639191058/https://lgtm.com/query/2373242062587923859/https://lgtm.com/query/1615647564237672449/