Page MenuHomePhabricator

[clang-tidy] CERT MSC24-C Obsolescent Functions check
Needs RevisionPublic

Authored by ktomi996 on Nov 7 2020, 3:24 AM.

Details

Summary

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

The checker warns only if __STDC_LIB_EXT1__ macro is defined and the value of __STDC_WANT_LIB_EXT1__ macro is 1 in this case it suggests the corresponding functions from Annex K instead of the vulnerable function.

https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

Diff Detail

Event Timeline

ktomi996 created this revision.Nov 7 2020, 3:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2020, 3:24 AM
ktomi996 requested review of this revision.Nov 7 2020, 3:24 AM
ktomi996 updated this revision to Diff 303633.Nov 7 2020, 3:37 AM
steakhal added a subscriber: steakhal.
lebedev.ri retitled this revision from MSC24-C Obsolescent Functions check to [clang-tidy] CERT MSC24-C Obsolescent Functions check.Nov 7 2020, 4:03 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp
19

Why this could not be member of check class?

52

Why not to use plain assignment? See readability-simplify-boolean-expr.

59

Early return will be better.

69

Please elide braces.

80

Please elide braces.

84

Could be const auto *, because type is spelled in same statement.

85

Could be const auto *, because type is spelled in same statement.

92

Could be const auto *, because type is spelled in same statement.

93

Could be const auto *, because type is spelled in same statement.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h
12

Please separate include statement and namespaces with empty line.

13

Please separate with empty line.

clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
6

Please make first statement same as in Release Notes. Please enclose all preprocessor variables and function names in double back-ticks.

15

Link to original rule should be at the end. See other documentation as example.

17

Please remove space before colon and separate code with empty line.

18

Should be .. code-block:: c++. See other documentation as formatting example.

clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp
5

Please separate with empty line.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.Nov 7 2020, 7:25 AM

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.
What do you think?

There is a mismatch between your code and docs too, regarding the function you check for.
In the code you match for vsprintf, vsscanf, vswprintf, vswcanf, wcrtomb, wcscat, but none of these are mentioned in the docs.

The checker warns only if STDC_LIB_EXT1 macro is defined and the value of STDC_WANT_LIB_EXT1 macro is 1 in this case it suggests the corresponding functions from Annex K instead the vulnerable function.

I would suggest mentioning these macros and their purpose in the docs.
Eg. that the STDC_WANT_LIB_EXT1 should be defined to 1 but the other is left to the implementation.

That being said, I would request more tests, demonstrating that this macro detection works accordingly.

This checker might be a bit noisy. Have you tried it on open-source projects?
If it is, we should probably note that in the docs as well.

In the tests, It is a good practice to demonstrate that the offered recommendation does not trigger yet another warning.
Don't forget to put a no-warning to highlight that.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp
30

Hm, it must be a typo.
vswcanf -> vswscanf

By the same token, I miss these functions from this enumeration.
vfwprintf, vfwscanf, vprintf, vwprintf, vwscanf
However, they are present in the cert table you mentioned.

33

Why is strlen an obsolescent function? I don't feel it justified, even if there was a comment about it on the cert page.

69

Please also check if the value is 1, as your summary and the standard requires.

77–79
87–89

We should probably use llvm::Twine instead of multiple std::string constructions.
I might be wrong though.

89

can -> should
The same at the other diagnostic message.

91

This and the previous if statement is exclusive, am I right?
If we have a match, that is either a callexpr or a vardecl.

I would prefer a return at the end of the previous if and an assert here that the FunctionPointer must be non-null.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h
27

It should be private.

clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
10

Another typo.

19–23

I'm not convinced by this example.

AFAIK, the intended usage is to define the __STDC_WANT_LIB_EXT1__ to 1 before the string.h header included.
It's important to provide a good example, as this checker will be silent if not used correctly.

23

We should probably align this comment as well.

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

What do you think?

There is a mismatch between your code and docs too, regarding the function you check for.
In the code you match for vsprintf, vsscanf, vswprintf, vswcanf, wcrtomb, wcscat, but none of these are mentioned in the docs.

The checker warns only if STDC_LIB_EXT1 macro is defined and the value of STDC_WANT_LIB_EXT1 macro is 1 in this case it suggests the corresponding functions from Annex K instead the vulnerable function.

I would suggest mentioning these macros and their purpose in the docs.
Eg. that the STDC_WANT_LIB_EXT1 should be defined to 1 but the other is left to the implementation.

That being said, I would request more tests, demonstrating that this macro detection works accordingly.

This checker might be a bit noisy. Have you tried it on open-source projects?
If it is, we should probably note that in the docs as well.

In the tests, It is a good practice to demonstrate that the offered recommendation does not trigger yet another warning.
Don't forget to put a no-warning to highlight that.

steakhal requested changes to this revision.Dec 23 2020, 4:38 AM

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.
IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

This revision now requires changes to proceed.Dec 23 2020, 4:38 AM

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

I don't see any value in disabling the check for eg. setbuf. Even the man page says that you should use the setvbuf instead.
Why would anyone disable this if one already wants diagnostics for the CERT rule it implements?

IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

I don't see any value in disabling the check for eg. setbuf. Even the man page says that you should use the setvbuf instead.
Why would anyone disable this if one already wants diagnostics for the CERT rule it implements?

IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

I think the question is, *why* are these checks being implemented?
Just to claim that for some particular rule there is a check, and cross it off a list?
Or for them to be actually used?

I think the question is, *why* are these checks being implemented?
Just to claim that for some particular rule there is a check, and cross it off a list?

Initially, yes. I think one could learn a lot from contributing to any project.
It's not inherently a bad thing to combine these two.

Or for them to be actually used?

I want a useful checker, that's why I highlighted some of my concerns and suggested a way forward.
It might not be useful to everyone as it tries to implement a domain-specific CERT rule, but it's still up to the user to enable this.
I think we should ask, who is the audience of this checker?
I assume only the users who are interested in the CERT guideline would use this. At this point, we should be clear about what we are checking for.
I think it's OK, to say that only a part of the rule is implemented, and we should carefully document this fact. But IMO one should go the extra mile to try hard and implement the other parts of the rule as well.
Like, matching on the setbuf is not that hard really. The rest of the missing functions probably fall into the same category.

I might be wrong on this though.
Keep in mind that I'm not tidy dev, so take my opinion with a pinch of salt.

I think the question is, *why* are these checks being implemented?
Just to claim that for some particular rule there is a check, and cross it off a list?
Or for them to be actually used?

We have an existing CERT module for CERT-specific checks. When the check is honoring a published coding standard, that's usually sufficient justification -- but I do think the check should implement what the CERT recommendation says. If the CERT recommendation seems deficient in some way, it's reasonable to reach out to the authors (they allow comments on their wiki and are pretty good about responding to those comments) for clarification or to discuss potential improvements to the recommendation that would make the check more palatable.

whisperity edited the summary of this revision. (Show Details)Tue, Jan 5, 2:41 AM
whisperity added a subscriber: gerazo.