[clang] [clang][analyzer] Add checker 'security.UnsafeSymlinkTest' (PR #221184)
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 21 06:40:23 PDT 2026
================
@@ -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",
----------------
balazske wrote:
The new warning is different, if too long it can be split into the "warning" and "note" parts.
https://github.com/llvm/llvm-project/pull/221184
More information about the cfe-commits
mailing list