This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy][Part1] Add a new module Android and three new checks.
ClosedPublic

Authored by yawanng on May 17 2017, 5:18 PM.

Details

Summary

A common source of security bugs is code that opens a file descriptors without using the O_CLOEXEC flag. (Without that flag, an opened sensitive file would remain open across a fork+exec to a lower-privileged SELinux domain, leaking that sensitive data.).

Add a new Android module and one checks in clang-tidy.

  • open(), openat(), and open64() should include O_CLOEXEC in their flags argument. [android-file-open-flag]

Links to part2 and part3:
https://reviews.llvm.org/D33745
https://reviews.llvm.org/D33747

Diff Detail

Event Timeline

yawanng created this revision.May 17 2017, 5:18 PM
yawanng edited the summary of this revision. (Show Details)May 17 2017, 5:19 PM
yawanng updated this revision to Diff 99374.May 17 2017, 5:25 PM

Add unit test file.

Please run clang-format over your code.

Please add documentation and mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Eugene.Zelenko set the repository for this revision to rL LLVM.
yawanng edited the summary of this revision. (Show Details)May 17 2017, 7:45 PM
srhines added inline comments.May 17 2017, 10:17 PM
clang-tidy/android/FileDescriptorCheck.cpp
65 ↗(On Diff #99374)

Use "%0 " instead of the 3 function names, so that you can use << to print the name as part of the diagnostic. You can get the name by binding the FunctionDecl in the matcher you create (and then that lets you do "FD->getName()"). You can look at something like FasterStringFindCheck.cpp for a good example of this (search for "%0").

76 ↗(On Diff #99374)

Using O_CLOEXEC here is potentially a problem, since most of our compiles are cross-compiles. If O_CLOEXEC is different on the target platform than the host platform (where this code is being compiled), this check would fail. Perhaps we can get the value of O_CLOEXEC as defined for the translation unit (and if it isn't defined, that's already a pretty big indicator that any use of these functions will be wrong). Alternately, maybe just re-parsing for "O_CLOEXEC" is better.

joerg added a subscriber: joerg.May 18 2017, 9:42 AM

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

yawanng updated this revision to Diff 99447.May 18 2017, 9:45 AM

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

yawanng marked an inline comment as done.May 18 2017, 10:30 AM
yawanng added inline comments.
clang-tidy/android/FileDescriptorCheck.cpp
76 ↗(On Diff #99374)

I also think re-parsing is better. I will think about this and update soon.

yawanng retitled this revision from [clang-tidy] Add a new module Android and a new check for file descriptors. to [WIP][clang-tidy] Add a new module Android and a new check for file descriptors. .May 18 2017, 10:34 AM
yawanng set the repository for this revision to rL LLVM.

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C. I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

clang-tidy/android/FileDescriptorCheck.cpp
66 ↗(On Diff #99447)

This can just pass FD instead of FD->getName() -- that will also properly quote the function call.

This diagnostic doesn't tell the user what's wrong with their code, just how to silence the warning. I'm not keen on "must" in this diagnostic either. We don't usually tell the user they must do something unless they truly must. We usually waffle a bit and use phrase like "<reasons why the code is bad>; consider using O_CLOEXEC instead".

alexfh edited edge metadata.May 18 2017, 11:43 AM

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C.

I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?

I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone".

clang-tidy/android/AndroidTidyModule.cpp
16

Are readability headers are really necessary?

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C.

I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?

If there are plans to add more checks, then yes. However, I think I'd prefer to see at least 2-3 checks in the work (or have some commitment for doing at least that many checks) before we add a module for it. I mostly worry about adding a single check and then nothing else. (No matter what module name we're talking about, btw.) I'd be fine with android, posix, or unix, depending on the nature of the checks.

I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone".

I was hesitant to suggest misc, but I was hoping to avoid adding a module with a single check under it and no commitment for further ones.

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C.

I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?

If there are plans to add more checks, then yes. However, I think I'd prefer to see at least 2-3 checks in the work (or have some commitment for doing at least that many checks) before we add a module for it. I mostly worry about adding a single check and then nothing else. (No matter what module name we're talking about, btw.) I'd be fine with android, posix, or unix, depending on the nature of the checks.

I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone".

I was hesitant to suggest misc, but I was hoping to avoid adding a module with a single check under it and no commitment for further ones.

There are also two other requirements(not yet implemented). There will be more checks following up.

alexfh requested changes to this revision.May 22 2017, 6:00 AM
alexfh added inline comments.
clang-tidy/android/FileDescriptorCheck.cpp
25 ↗(On Diff #99447)

functionDecl() is implicitly allOf.

26 ↗(On Diff #99447)

isExpansionInFileMatching(...) is a dependency on the implementation of the library. The fact that #include <fcntl.h> provides this macro doesn't mean the macro is defined in this header (it could be in another transitively included header). Same below.

26 ↗(On Diff #99447)

asString("int") is wasteful, since it needs to allocate the string and print the return type name there. hasType(isInteger()) is a much better alternative.

28 ↗(On Diff #99447)

matchesName is wasteful, if you need just to compare the name. You probably want to use hasAnyName("open", "open64").

90 ↗(On Diff #99447)
test/clang-tidy/android-file-descriptor.cpp
3 ↗(On Diff #99447)

We don't use actual library headers in the tests. Instead, please add mock declarations of the API you need (open, open64, O_ flags, etc.).

This revision now requires changes to proceed.May 22 2017, 6:00 AM

I will make some major changes to this CL based on the current suggestions from reviewers and update it for further review later. Thank you for the valuable advice.

It's also necessary to mention new checks group in docs/clang-tidy/index.rst.

yawanng updated this revision to Diff 100777.May 30 2017, 2:43 PM
yawanng edited edge metadata.

Modify the detection algorithm. Add another two checks and corresponding tests as well as the docs.

yawanng retitled this revision from [WIP][clang-tidy] Add a new module Android and a new check for file descriptors. to [clang-tidy] Add a new module Android and a new check for file descriptors. .May 30 2017, 2:45 PM
yawanng edited the summary of this revision. (Show Details)

Hi, I have updated this CL for review. Thank you very much :)

Eugene.Zelenko added inline comments.May 30 2017, 2:51 PM
clang-tidy/utils/ExprToStr.cpp
23 ↗(On Diff #100777)

Please add empty line.

clang-tidy/utils/ExprToStr.h
22 ↗(On Diff #100777)

Please add empty line.

docs/ReleaseNotes.rst
69

May be detect usage of `creat()` will be better?

docs/clang-tidy/checks/android-creat-usage.rst
5 ↗(On Diff #100777)

Please enclose creat() and open() in `` and add empty line before.

docs/clang-tidy/checks/android-file-open-flag.rst
6

Please add empty line.

11

Unnecessary empty line.

docs/clang-tidy/checks/android-fopen-mode.rst
5 ↗(On Diff #100777)

Please add empty line.

yawanng updated this revision to Diff 100782.May 30 2017, 2:52 PM
yawanng updated this revision to Diff 100786.May 30 2017, 3:05 PM
yawanng marked 8 inline comments as done.
yawanng retitled this revision from [clang-tidy] Add a new module Android and a new check for file descriptors. to [clang-tidy] Add a new module Android and three new checks..May 30 2017, 4:00 PM

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

yawanng edited the summary of this revision. (Show Details)May 31 2017, 10:27 AM

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

The other two checks are relatively small and simple. I understand this may make the review process longer and we could wait for that. Please take your time :-) Thank you!

yawanng edited the summary of this revision. (Show Details)May 31 2017, 10:35 AM

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

The other two checks are relatively small and simple. I understand this may make the review process longer and we could wait for that. Please take your time :-) Thank you!

It's not only longer to review larger patches, it's also less convenient for the reviewers. Please send each check as a separate patch (the addition of the module can be in one of them). Phabricator has decent support for dependent patches, so it shouldn't add much work on your side.

yawanng updated this revision to Diff 100928.May 31 2017, 3:34 PM

Split the commit to three ones that each of them contains one. This is the first part.

yawanng retitled this revision from [clang-tidy] Add a new module Android and three new checks. to [clang-tidy][Part1] Add a new module Android and three new checks..May 31 2017, 3:34 PM
yawanng updated this revision to Diff 100934.May 31 2017, 4:06 PM
yawanng edited the summary of this revision. (Show Details)
yawanng edited the summary of this revision. (Show Details)May 31 2017, 4:33 PM
hokein edited edge metadata.Jun 2 2017, 3:46 AM

Thanks for the contributions.

All your three checks are not android specific -- because they are checking POSIX APIs (open, creat, fopen), which are more likely related to POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but wait to see other reviewers' opinions before renaming it.

jbcoe added a subscriber: jbcoe.Jun 2 2017, 4:58 AM
alexfh added a comment.Jun 2 2017, 9:27 AM

Thanks for the contributions.

All your three checks are not android specific -- because they are checking POSIX APIs (open, creat, fopen), which are more likely related to POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but wait to see other reviewers' opinions before renaming it.

IIUC, these checks enforce a certain - Android-specific - way of using POSIX APIs. I'm not sure if the recommendations are universally useful. Or am I mistaken?

yawanng updated this revision to Diff 101267.Jun 2 2017, 1:42 PM
yawanng updated this revision to Diff 101279.Jun 2 2017, 2:33 PM

Format changes.

hokein added a comment.Jun 7 2017, 9:41 AM

IIUC, these checks enforce a certain - Android-specific - way of using POSIX APIs. I'm not sure if the recommendations are universally useful. Or am I mistaken?

OK, that makes sense. I may miss some background context.

docs/clang-tidy/index.rst
58

Add some words explaining this module?

yawanng updated this revision to Diff 101831.Jun 7 2017, 3:28 PM
yawanng marked an inline comment as done.
alexfh requested changes to this revision.Jun 9 2017, 8:39 AM
alexfh added inline comments.
clang-tidy/android/FileOpenFlagCheck.cpp
61

No need for this variable, since it's just used once. Same below.

63–66

No need to replace the text with itself. Just insert the part that is missing. The more local the changes - the fewer are chances of conflicts.

clang-tidy/android/FileOpenFlagCheck.h
39

This doesn't have to be a class member. It could be local to the function where it's used or to the .cc file.

docs/clang-tidy/checks/android-file-open-flag.rst
7

I'm not sure about using of "has been" here. Maybe just use present tense? You might want to consult a nearby native speaker though. Alternatively, you might want to rephrase this as "Opening files using POSIX `open or similar system calls without specifying the O_CLOEXEC` flag is a common source of security bugs."

10

Missing period before "Functions".

10

Just "functions" is too broad. I'd say "open-like functions", "POSIX functions that open files" or something like that.

This revision now requires changes to proceed.Jun 9 2017, 8:39 AM
yawanng updated this revision to Diff 102064.Jun 9 2017, 12:15 PM
yawanng edited edge metadata.
yawanng marked 6 inline comments as done.
yawanng added inline comments.Jun 13 2017, 10:02 AM
docs/clang-tidy/checks/android-file-open-flag.rst
7

I copied these sentences from the requests written by a native speaker, I think. But after talking with another native speaker, yes, present tense seems to be better here. Thanks :)

yawanng edited the summary of this revision. (Show Details)Jun 13 2017, 10:29 AM
hokein added inline comments.Jun 14 2017, 8:23 AM
clang-tidy/android/FileOpenFlagCheck.cpp
29

I'd put the hasAnyName matcher in front of hasParameter which follows the order of function declaration.

The same below.

47

How about

const Expr *FlagArg = nullptr;
if (const auto* OpenFnCall = Result.Nodes.getXXX()) {
  // ...
} else if (const auto* OpenAtFnCall = Result.Nodes.getXX()) {
  // ...
}
assert(FlagArg);

?

62

Instead of using getLangOpts(), you should use Result.Context.getLangOpts().

68

No need to be a class member method, put it inside this translation unit.

80

You could use Lexer::getImmediateMacroName(Loc, SM, Opts); to get the marco name, or doesn't it work here?

85

I think you can use early return here. With that you don't need to maintain the flag variable IsFlagIn, so the code structure like:

if (isa<..>) {
   
    return ...;
}

if (isa<BinaryOperator>()) {
    // ...
    return ...;
}
retrun false;
89

You can use if (const auto* BO = dyn_cast<BinaryOperator>(Flags)), so that you don't call cast<BinaryOperator>(Flags) multiple times below.

test/clang-tidy/android-file-open-flag.cpp
77

I would add tests where O_CLOEXEC is not at the end of parameter 2 expression like open("filename", O_RDWR | O_CLOEXEC | O_EXCL).

yawanng updated this revision to Diff 102577.Jun 14 2017, 10:45 AM
yawanng marked 7 inline comments as done.
yawanng added inline comments.
clang-tidy/android/FileOpenFlagCheck.cpp
62

May I ask what's the difference between the Result.Context.getLangOpts() and getLangOpts()? Does Result.Context.getLangOpts() have a longer lifetime than getLangOpts()?

80

getImmediateMacroName sometimes cannot return the O_CLOEXEC , like

#define O_CLOEXEC _O_CLOEXEC
#define _O_CLOEXEC 1

getImmediateMacroName will return _O_CLOEXEC, whereas O_CLOEXEC is what we need. I change this to directly get the source text if it's a macro, which seems to be simpler.

yawanng updated this revision to Diff 102690.Jun 15 2017, 10:48 AM

Format change.

hokein accepted this revision.Jun 19 2017, 1:13 AM

The code looks good to me now, I'd wait to see whether @alexfh has further comments before submitting the patch. Thanks for your patience with the review ;)

clang-tidy/android/FileOpenFlagCheck.cpp
22

checkFlags name is a bit of ambiguous, maybe rename it to "HasCloseOnExecFlag"?

Notice that you use the string "O_CLOEXEC" several times in the code, I'd suggest to use a "CloseOnExecFlag" variable.

33

Nit: The outermost () is not needed.

62

IMO they are the same fundamentally, except Result.Context.getLangOpts() can save a copy. We usually use Result.Context.getLangOpts() inside ClangTidyCheck::check(const MatchFinder::MatchResult &Result).

80

Ah, I see, thanks for the clarification!

yawanng updated this revision to Diff 103068.Jun 19 2017, 9:57 AM
yawanng marked 3 inline comments as done.
yawanng marked an inline comment as done.
Prazek removed a subscriber: Prazek.Jun 19 2017, 1:58 PM
alexfh accepted this revision.Jun 23 2017, 4:21 AM

LG with one nit.

clang-tidy/android/FileOpenFlagCheck.cpp
25

nit: "Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())." (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

This revision is now accepted and ready to land.Jun 23 2017, 4:21 AM
yawanng closed this revision.Jun 23 2017, 2:38 PM
yawanng marked an inline comment as done.Jun 23 2017, 3:33 PM