llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Kéri (balazske) <details> <summary>Changes</summary> --- Patch is 41.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/221184.diff 6 Files Affected: - (modified) clang/docs/analyzer/checkers.rst (+114) - (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4) - (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (+1) - (added) clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp (+541) - (added) clang/test/Analysis/unsafe-symlink-test-notes.c (+115) - (added) clang/test/Analysis/unsafe-symlink-test.c (+412) ``````````diff diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 1f6b974d5ca7a..77337274e70ea 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -2048,6 +2048,120 @@ this) and always check the return value of these calls. This check corresponds to SEI CERT Rule `POS36-C <https://wiki.sei.cmu.edu/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges>`_. +security.UnsafeSymlinkTest (C, C++) +""""""""""""""""""""""""""""""""""" + +Check unsafe detection of symbolic links. + +The following code is not a safe way to detect a symbolic link. The file can be +manipulated asynchronously between the call to ``lstat`` and ``open`` and data +in ``fs`` may become outdated: + +.. code-block:: c + + void handle_file(const char *filename) { + struct stat fs; + int fd; + + if (lstat(filename, &fs) == -1) + return; + + if (!S_ISLNK(fs.st_mode)) { + fd = open(filename, O_RDWR); // warning: inaccurate check for symbolic link status of file + if (fd == -1) + return; + } + // ... + } + +The checker produces a warning in similar cases when a file is opened after the +``stat`` data was obtained for it and presence of symbolic link was checked by +macro ``S_ISLNK``. + +A secure way is to use the ``O_NOFOLLOW`` value in the ``flags`` argument at +``open``. If this flag is not available on the implementation, the file status +can be obtained a second time after the ``open`` call. If there is no difference +between this data and the previously (before open) obtained data, the presence +of symbolic link can be checked in a safe way. + +.. code-block:: c + + void write_nosymlink(const char *filename, const char *buf, size_t size) { + struct stat stat1; + int fd; + + if (lstat(filename, &stat1) == -1) + return; + + fd = open(filename, 1); + if (fd == -1) + return; + + struct stat stat2; + if (fstat(fd, &stat2) == -1) { + // error: fstat failed + // ... + return; + } + + if (stat1.st_mode != stat2.st_mode || stat1.st_ino != stat2.st_ino || stat1.st_dev != stat2.st_dev) { + // error: file was changed + // ... + return; + } + + if (S_ISLNK(stat1.st_mode)) { + // file is a symbolic link + // ... + return; + } + + write(fd, buf, size); + // ... + } + +It is important to compare all fields ``st_mode``, ``st_ino`` and ``st_dev`` of +the ``stat`` structure. This checker emits additionally a warning if a file +write or read attempt is made in a similar case when these comparisons are +incomplete (or missing). + +.. code-block:: c + + void write_nosymlink(const char *filename, const char *buf, size_t size) { + struct stat stat1; + int fd; + + if (lstat(filename, &stat1) == -1) + return; + + fd = open(filename, 1); + if (fd == -1) + return; + + struct stat stat2; + if (fstat(fd, &stat2) == -1) { + // ... + return; + } + + if (stat1.st_mode != stat2.st_mode) { // missing comparison of st_ino and st_dev + // ... + return; + } + + if (S_ISLNK(stat1.st_mode)) { + // ... + return; + } + + write(fd, buf, size); // warning: possibly missing check for external change of file before it was opened + // ... + } + +This kind of warning is produced when a ``lstat`` - ``open`` - ``fstat`` call +sequence is found for the same file before write or read attempt (and the +comparisons of status data are missing). + .. _security-VAList: security.VAList (C, C++) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index b6b3857dc7b35..588850a79111e 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1007,6 +1007,10 @@ let ParentPackage = Security in { "'setuid(getuid())' (CERT: POS36-C)">, Documentation<HasDocumentation>; + def UnsafeSymlinkTestChecker : Checker<"UnsafeSymlinkTest">, + HelpText<"Check unsafe detection of symbolic links">, + Documentation<HasDocumentation>; + def VAListChecker : Checker<"VAList">, HelpText<"Warn on misuse of va_list objects">, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index befd60dbec542..fbb7c3b838085 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -128,6 +128,7 @@ add_clang_library(clangStaticAnalyzerCheckers UninitializedObject/UninitializedPointee.cpp UnixAPIChecker.cpp UnreachableCodeChecker.cpp + UnsafeSymlinkTestChecker.cpp UseAfterLifetimeEnd.cpp VforkChecker.cpp VLASizeChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp new file mode 100644 index 0000000000000..a5a0c050ebc12 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp @@ -0,0 +1,541 @@ +//===-- UnsafeSymlinkTestChecker.cpp ------------------------------*- C++ -*--// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// Defines a checker that checks for unsafe symlink detection. This checks for +// 2 related conditions: +// - File status is read and used to detect symlink before the file is opened. +// The file can be changed asynchronously between reading the status data and +// opening the file, so this check is not safe to use. +// - To fix the previous issue, the file status can be read after the open too +// and compared to the previous value. If it did not change, the symlink +// status is safely determined (the file can not be changed externally after +// it was opened). The checker can detect a missing comparison of the "before" +// and "after" status values. +// (In all cases use of the O_NOFOLLOW flag at 'open' prevents the warning.) +// +//===----------------------------------------------------------------------===// + +#include "clang/AST/StmtVisitor.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h" +#include <optional> + +using namespace clang; +using namespace ento; + +namespace { + +/// Used to identify a file name. +/// If created with a symbolic region, use the region as key. +/// If created with a string region, use the contained string as key (different +/// string regions with same content should be equal). +struct FileNameKey { + std::string FileNameStr; + const MemRegion *Region = nullptr; + + FileNameKey(const MemRegion *R) { + R = R->StripCasts(); + if (const auto *SR = dyn_cast<StringRegion>(R)) + FileNameStr = SR->getStringLiteral()->getString(); + else + Region = R; + } + + void Profile(llvm::FoldingSetNodeID &ID) const { + ID.AddString(FileNameStr); + ID.AddPointer(Region); + } + + bool operator==(const FileNameKey &RHS) const { + return FileNameStr == RHS.FileNameStr && Region == RHS.Region; + } + + bool operator<(const FileNameKey &RHS) const { + if (!Region && !RHS.Region) + return FileNameStr < RHS.FileNameStr; + return Region < RHS.Region; + } + + std::string getFileName(llvm::StringRef PrefixStr) const { + if (!Region) + return (llvm::Twine(PrefixStr) + "'" + FileNameStr + "'").str(); + return ""; + } +}; + +/// Data maintained about a region belonging to a "struct stat". +struct StatData { + /// Region of a 'struct stat' object. + const SubRegion *Region; + /// Value of the field 'st_mode'. + SVal StModeVal; + /// Value of the field 'st_ino'. + SVal StInoVal; + /// Value of the field 'st_dev'. + SVal StDevVal; + + bool operator==(const StatData &D) const { + return Region == D.Region && StModeVal == D.StModeVal && + StInoVal == D.StInoVal && StDevVal == D.StDevVal; + } + + void Profile(llvm::FoldingSetNodeID &ID) const { + ID.AddPointer(Region); + StModeVal.Profile(ID); + StInoVal.Profile(ID); + StDevVal.Profile(ID); + } +}; + +/// Data about a file after `lstat` (but not `open`) was called. +struct FileDataLStat { + /// Information about the `stat` structure that was passed to `lstat`. + StatData LStatD; + /// Indicates if a test for symbolic link on the `st_mode` field of the `stat` + /// structure was performed, using the `S_ISLNK` macro. + bool LinkCheckPerformed; + + void Profile(llvm::FoldingSetNodeID &ID) const { + LStatD.Profile(ID); + ID.AddBoolean(LinkCheckPerformed); + } + + bool operator==(const FileDataLStat &R) const { + return LStatD == R.LStatD && LinkCheckPerformed == R.LinkCheckPerformed; + } +}; + +/// Data about a file after `lstat` and `open` was called (no symbolic link test +/// with `S_ISLNK` was performed in between). +struct FileDataOpened { + /// Information about the `stat` structure that was passed to `lstat`. + StatData LStatD; + /// Information about the `stat` structure that was passed to `fstat`. + StatData FStatD; + /// Data about the file name (this is used for checker messages). + FileNameKey FName; + + void Profile(llvm::FoldingSetNodeID &ID) const { + LStatD.Profile(ID); + FStatD.Profile(ID); + } + + bool operator==(const FileDataOpened &R) const { + return LStatD == R.LStatD && FStatD == R.FStatD; + } +}; + +struct StatFieldsDecl { + const FieldDecl *StModeFD; + const FieldDecl *StInoFD; + const FieldDecl *StDevFD; + + bool isValid() const { return StModeFD && StInoFD && StDevFD; } +}; + +struct ASTData { + const FieldDecl *StModeFD; + const FieldDecl *StInoFD; + const FieldDecl *StDevFD; + QualType StructStatType; + int64_t O_NOFOLLOWValue; + bool IsValid; + void checkValid() { + IsValid = StModeFD && StInoFD && StDevFD && !StructStatType.isNull(); + } +}; + +class UnsafeSymlinkTestChecker + : public Checker<check::PostCall, check::BranchCondition, + check::RegionChanges, check::DeadSymbols> { + const CallDescription LStatFn{CDM::CLibrary, {"lstat"}, 2}; + const CallDescription OpenFn{CDM::CLibrary, {"open"}, 2}; + const CallDescription FStatFn{CDM::CLibrary, {"fstat"}, 2}; + const CallDescriptionSet FileAccessFn{ + {CDM::CLibrary, {"write"}, 3}, {CDM::CLibrary, {"writev"}, 3}, + {CDM::CLibrary, {"pwrite"}, 4}, {CDM::CLibrary, {"read"}, 3}, + {CDM::CLibrary, {"readv"}, 3}, {CDM::CLibrary, {"pread"}, 4}, + {CDM::CLibrary, {"lseek"}, 3}}; + + const BugType BT{this, "Security error", "Incorrect check for symbolic link", + false}; + + mutable std::optional<ASTData> ASTValues; + +public: + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + void checkBranchCondition(const Stmt *S, CheckerContext &C) const; + ProgramStateRef checkRegionChanges(ProgramStateRef State, + const InvalidatedSymbols *Invalidated, + ArrayRef<const MemRegion *> Explicits, + ArrayRef<const MemRegion *> Regions, + const StackFrame *SF, + const CallEvent *Call) const; + void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; + +private: + const SubRegion *castRegionToStructStat(const MemRegion *R, + CheckerContext &C) const { + if (!R) + return nullptr; + std::optional<const MemRegion *> CastR = C.getStoreManager().castRegion( + R, C.getASTContext().getPointerType(ASTValues->StructStatType)); + if (!CastR) + return R->getAs<SubRegion>(); + const SubRegion *SR = (*CastR)->getAs<SubRegion>(); + return SR ? SR : R->getAs<SubRegion>(); + } + StatData getStatData(const SubRegion *StatR, ProgramStateRef State, + CheckerContext &C) const { + MemRegionManager &RM = C.getStoreManager().getRegionManager(); + auto *StatR1 = castRegionToStructStat(StatR, C); + auto GetFieldSVal = [&](const FieldDecl *FD) { + return State->getSVal(RM.getFieldRegion(FD, StatR1)); + }; + return {StatR, GetFieldSVal(ASTValues->StModeFD), + GetFieldSVal(ASTValues->StInoFD), GetFieldSVal(ASTValues->StDevFD)}; + } + const NoteTag *getNoteTag(const MemRegion *R, std::string Message, + CheckerContext &C) const; + void initData(const RecordDecl *StatDecl, const Preprocessor &PP) const; +}; + +} // end anonymous namespace + +/// Data about files where `lstat` was called but not `open`. +REGISTER_MAP_WITH_PROGRAMSTATE(LStatCalledMap, FileNameKey, FileDataLStat) + +/// Data about files where `lstat` and `open` was called. +REGISTER_MAP_WITH_PROGRAMSTATE(LStatOpenCalledMap, SymbolRef, FileDataOpened) + +const NoteTag *UnsafeSymlinkTestChecker::getNoteTag(const MemRegion *R, + std::string Message, + CheckerContext &C) const { + return C.getNoteTag( + [this, R, Message](PathSensitiveBugReport &BR) -> std::string { + if (BR.isInteresting(R) && &BR.getBugType() == &BT) + return Message; + return ""; + }); +} + +static const FieldDecl *findField(llvm::StringRef FieldName, + const RecordDecl *RD) { + auto FoundField = + llvm::find_if(RD->fields(), [&FieldName](const FieldDecl *F) { + return F->getNameAsString() == FieldName; + }); + if (FoundField == RD->fields().end()) + return nullptr; + return *FoundField; +} + +void UnsafeSymlinkTestChecker::initData(const RecordDecl *StatDecl, + const Preprocessor &PP) const { + if (StatDecl) { + ASTValues = {findField("st_mode", StatDecl), + findField("st_ino", StatDecl), + findField("st_dev", StatDecl), + StatDecl->getASTContext().getCanonicalTagType(StatDecl), + 0, + false}; + if (std::optional<int> Val = tryExpandAsInteger("O_NOFOLLOW", PP)) + ASTValues->O_NOFOLLOWValue = *Val; + } else { + ASTValues = {nullptr}; + } + ASTValues->checkValid(); +} + +void UnsafeSymlinkTestChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + if (ASTValues && !ASTValues->IsValid) + return; + + ProgramStateRef State = C.getState(); + + if (LStatFn.matches(Call)) { + if (!ASTValues) { + initData( + Call.parameters()[1]->getType()->getPointeeType()->getAsRecordDecl(), + C.getPreprocessor()); + if (!ASTValues->IsValid) + return; + } + + const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion(); + const auto *StatReg = + dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion()); + if (!FNameReg || !StatReg) + return; + + FileNameKey FName(FNameReg); + State = State->set<LStatCalledMap>(FName, + {getStatData(StatReg, State, C), false}); + C.addTransition(State, getNoteTag(StatReg, + (llvm::Twine("File status") + + FName.getFileName(" of file ") + + " is read here before opening the file") + .str(), + C)); + return; + } + + if (OpenFn.matches(Call)) { + const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion(); + FileNameKey FName(FNameReg); + const FileDataLStat *LStatData = State->get<LStatCalledMap>(FName); + SymbolRef FileDescSym = Call.getReturnValue().getAsSymbol(); + if (!FNameReg || !LStatData || !FileDescSym) + return; + + State = State->remove<LStatCalledMap>(FNameReg); + + if (ASTValues->O_NOFOLLOWValue != 0) { + const llvm::APSInt *FlagsValue = + C.getSValBuilder().getKnownValue(State, Call.getArgSVal(1)); + if (!FlagsValue) { + C.addTransition(State); + return; + } + if (std::optional<int64_t> FVal = FlagsValue->tryExtValue(); + FVal && (*FVal & ASTValues->O_NOFOLLOWValue)) { + C.addTransition(State); + return; + } + } + + if (!LStatData->LinkCheckPerformed) { + State = State->set<LStatOpenCalledMap>( + FileDescSym, + {LStatData->LStatD, {nullptr, SVal{}, SVal{}, SVal{}}, FName}); + } else { + if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { + auto R = std::make_unique<PathSensitiveBugReport>( + BT, + (llvm::Twine("Inaccurate check for symbolic link status of file") + + FName.getFileName(" ")) + .str(), + N); + R->addNote("The file can be manipulated externally between calling " + "'lstat' and opening the file", + {Call.getSourceRange().getBegin(), C.getSourceManager()}); + R->addRange(Call.getSourceRange()); + R->markInteresting(LStatData->LStatD.Region); + C.emitReport(std::move(R)); + return; + } + } + } + + if (FStatFn.matches(Call)) { + SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol(); + const auto *FStatReg = + dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion()); + if (!FileDescSym || !FStatReg) + return; + const FileDataOpened *FileData = + State->get<LStatOpenCalledMap>(FileDescSym); + if (!FileData) + return; + State = State->set<LStatOpenCalledMap>( + FileDescSym, + {FileData->LStatD, getStatData(FStatReg, State, C), FileData->FName}); + C.addTransition(State, + getNoteTag(FStatReg, + (llvm::Twine("File status") + + FileData->FName.getFileName(" of file ") + + " is read here after opening the file") + .str(), + C)); + return; + } + + if (FileAccessFn.contains(Call)) { + SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol(); + if (!FileDescSym) + return; + const FileDataOpened *FileData = + State->get<LStatOpenCalledMap>(FileDescSym); + if (!FileData) + return; + State = State->remove<LStatOpenCalledMap>(FileDescSym); + if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { + auto R = std::make_unique<PathSensitiveBugReport>( + BT, + (llvm::Twine("Possibly missing check for external change of file") + + FileData->FName.getFileName(" ")) + .str(), + N); + R->addNote( + "File status was obtained before and after opening the file which " + "indicates possible intent of a safe check for symbolic link", + {Call.getSourceRange().getBegin(), C.getSourceManager()}); + R->addNote("For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' " + "before and after open should be checked for equality", + {Call.getSourceRange().getBegin(), C.getSourceManager()}); + R->addRange(Call.getSourceRange()); + R->markInteresting(FileData->LStatD.Region); + R->markInteresting(FileData->FStatD.Region); + C.emitReport(std::move(R)); + return; + } + } + + C.addTransition(State); +} + +namespace { +class FindMacroVisitor : public ConstStmtVisitor<FindMacroVisitor, bool> { + const CheckerContext &C; + ProgramStateRef State; + const MemRegion *LStatInfoStModeReg; + + bool VisitChildren(const Stmt *S) { + for (const Stmt *Child : S->children()) + if (Child && Visit(Child)) + return true; + retu... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/221184 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
