================ @@ -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; ---------------- balazske wrote:
It is possible that `O_NOFOLLOW` does not exist at all, and the check should not be skipped in this case. But if it exists (and is found) and the flag has unknown value, it may contain `O_NOFOLLOW` and a warning could be false positive. Probably it is possible to check only the presence of `O_NOFOLLOW` macro (if the numerical value is not found) and skip the warning, I am not sure if this is a better option. https://github.com/llvm/llvm-project/pull/221184 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
