Skip to content

Add more safety checks to variadic function calls in checked scope #1182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mgrang merged 7 commits into from
Sep 10, 2021
Merged

Add more safety checks to variadic function calls in checked scope #1182

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b1a176f
Add more safety checks to variadic functions calls in checked scope
Sep 9, 2021
5fd3ed0
Added a couple of comments
Sep 9, 2021
7faaadf
Merge branch 'master' into varargs3
Sep 9, 2021
1d7515b
Add issue
Sep 9, 2021
55b89e3
Update clang/lib/Sema/SemaChecking.cpp
Sep 9, 2021
f71b669
Fix windows test
Sep 9, 2021
a579903
f
Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11530,6 +11530,9 @@ def err_bounds_type_annotation_lost_checking : Error<
def err_checked_scope_scanf_width : Error<
"in a checked scope width is not allowed with format specifier in scanf">;

def err_checked_scope_disallowed_format_specifier : Error<
"in a checked scope %0 format specifier is not allowed %1">;

def err_pragma_pop_checked_scope_mismatch : Error<
"#pragma CHECKED_SCOPE pop with no matching #pragma CHECKED_SCOPE push">;

Expand Down
85 changes: 71 additions & 14 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7581,10 +7581,13 @@ class CheckFormatHandler : public analyze_format_string::FormatStringHandler {

void HandleNullChar(const char *nullCharacter) override;

// Note: IsFuncScanfLike = true means that the variadic function being called
// is scanf-like. IsFuncScanfLike = false means that the function is
// printf-like.
void CheckVarargsInCheckedScope(
const analyze_format_string::ConversionSpecifier &CS,
const char *StartSpecifier, unsigned SpecifierLen, const Expr *E,
SmallString<128> FSString);
SmallString<128> FSString, bool IsFuncScanfLike = false);

template <typename Range>
static void
Expand Down Expand Up @@ -7945,7 +7948,7 @@ CheckFormatHandler::CheckNumArgs(
void CheckFormatHandler::CheckVarargsInCheckedScope(
const analyze_format_string::ConversionSpecifier &CS,
const char *StartSpecifier, unsigned SpecifierLen, const Expr *E,
SmallString<128> FSString) {
SmallString<128> FSString, bool IsFuncScanfLike) {

// Check arguments to variadic functions like printf/scanf, etc in checked
// scope. This function is called per argument. E is current argument that
Expand All @@ -7958,34 +7961,87 @@ void CheckFormatHandler::CheckVarargsInCheckedScope(
return;

QualType ArgTy = E->getType();
bool EmitVariadicFuncDiag = false;
std::string ExpectedTyMsg;

switch (CS.getKind()) {
default:
// In scanf-like functions, only allow _Ptr type arguments.
if (IsFuncScanfLike) {
if (ArgTy->isCheckedPointerNtArrayType() ||
ArgTy->isNtCheckedArrayType() ||
ArgTy->isCheckedPointerArrayType() ||
ArgTy->isCheckedArrayType()) {
EmitFormatDiagnostic(
S.PDiag(diag::err_checked_scope_invalid_format_specifier_argument)
<< FSString << "_Ptr",
E->getExprLoc(), /*IsStringLocation*/false,
getSpecifierRange(StartSpecifier, SpecifierLen));
}

// In printf-like functions, only allow scalar type arguments with format
// specifiers other than %s.

// TODO: Currently, we do not handle the case where an out-of-bounds
// null-terminated array is passed as an argument to %s. The following code
// is allowed even though it might be a safety hole.
// _Checked {
// _Nt_array_ptr<char> p : count(5);
// printf("%s", p + 1234);
// }
// Issue https://github.com/microsoft/checkedc-clang/pull/1182 tracks this.
Comment thread
mgrang marked this conversation as resolved.
Outdated

} else {
if (ArgTy->isCheckedPointerType()) {
EmitFormatDiagnostic(
S.PDiag(diag::err_checked_scope_invalid_format_specifier_argument)
<< FSString << "scalar",
E->getExprLoc(), /*IsStringLocation*/false,
getSpecifierRange(StartSpecifier, SpecifierLen));
}
}
break;

// Check if the argument corresponding to the %s format specifier is either
// _Nt_array_ptr or _Nt_checked.
case ConversionSpecifier::sArg:
if (!ArgTy->isCheckedPointerNtArrayType() &&
!ArgTy->isNtCheckedArrayType()) {
EmitVariadicFuncDiag = true;
ExpectedTyMsg = "null-terminated";
if (IsFuncScanfLike) {
EmitFormatDiagnostic(
S.PDiag(diag::err_checked_scope_disallowed_format_specifier)
<< FSString << "in scanf",
E->getExprLoc(), /*IsStringLocation*/false,
getSpecifierRange(StartSpecifier, SpecifierLen));

} else if (!ArgTy->isCheckedPointerNtArrayType() &&
!ArgTy->isNtCheckedArrayType()) {
EmitFormatDiagnostic(
S.PDiag(diag::err_checked_scope_invalid_format_specifier_argument)
<< FSString << "null-terminated",
E->getExprLoc(), /*IsStringLocation*/false,
getSpecifierRange(StartSpecifier, SpecifierLen));
}
break;
}

if (EmitVariadicFuncDiag) {
// Disallow %p format specifier with scanf-like functions.
case ConversionSpecifier::pArg:
if (IsFuncScanfLike) {
EmitFormatDiagnostic(
S.PDiag(diag::err_checked_scope_disallowed_format_specifier)
<< FSString << "with scanf",
E->getExprLoc(), /*IsStringLocation*/false,
getSpecifierRange(StartSpecifier, SpecifierLen));
}
break;

// Disallow %n specifier in checked scope.
case ConversionSpecifier::nArg:
EmitFormatDiagnostic(
S.PDiag(diag::err_checked_scope_invalid_format_specifier_argument)
<< FSString << ExpectedTyMsg,
S.PDiag(diag::err_checked_scope_disallowed_format_specifier)
<< FSString << "",
E->getExprLoc(), /*IsStringLocation*/false,
getSpecifierRange(StartSpecifier, SpecifierLen));
break;
}
}


template<typename Range>
void CheckFormatHandler::EmitFormatDiagnostic(PartialDiagnostic PDiag,
SourceLocation Loc,
Expand Down Expand Up @@ -9176,7 +9232,8 @@ bool CheckScanfHandler::HandleScanfSpecifier(
SmallString<128> FSString;
llvm::raw_svector_ostream os(FSString);
FS.toString(os);
CheckVarargsInCheckedScope(CS, startSpecifier, specifierLen, Ex, FSString);
CheckVarargsInCheckedScope(CS, startSpecifier, specifierLen, Ex, FSString,
/*IsFuncScanfLike*/ true);

analyze_format_string::ArgType::MatchKind Match =
AT.matchesType(S.Context, Ex->getType());
Expand Down
105 changes: 92 additions & 13 deletions clang/test/CheckedC/checked-scope/variadic-functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,25 @@
// variadic-functions-win.c and the non-windows tests are in
// variadic-functions-non-win.c.

// The checking of variadic functions in checked scope follows these rules:
// 1. All warnings issued by the -Wformat family of flags are errors in checked
// scope.
// 2. No bounds checking of arguments to variadic functions like printf/scanf,
// etc is done.

// For printf-like functions:
// 3. %s is allowed only with arg type _Nt_array_ptr or _Nt_checked.
// 4. %p is allowed with any arg type.
// 5. %n is disallowed.
// 6. For all other format specifiers, only scalar arg types are allowed.

// For scanf-like functions:
// 7. %s is disallowed.
// 8. All width modifiers to format specifiers are disallowed.
// 9. %p is disallowed
// 10. %n is disallowed
// 11. For all other format specifiers, only _Ptr arg types are allowed.

// RUN: %clang_cc1 -fcheckedc-extension -verify \
// RUN: -verify-ignore-unexpected=note %s

Expand Down Expand Up @@ -57,41 +76,96 @@ void f1(_Nt_array_ptr<char> chk, char *unchk1, _Array_ptr<char> arr) {
int a = 1, b = 2;
char *unchk2;

// Calling printf-scanf-like functions with unchecked pointers is allowed in
// unchecked scope.
printf("%d %s %d %s", 1, unchk1, 2, unchk2);
MyPrintf("%d %s %d %s", 1, unchk1, 2, unchk2);
scanf("%d %s %d %s", &a, unchk1, &b, unchk2);
MyScanf("%d %s %d %s", &a, unchk1, &b, unchk2);

_Checked {
// Calling printf-scanf-like functions with unchecked pointers is disallowed
// in checked scope.
printf("%d %s %d %s", 1, unchk1, 2, unchk2); // expected-error {{local variable used in a checked scope must have a checked type}} expected-error {{parameter used in a checked scope must have a checked type or a bounds-safe interface}}
MyPrintf("%d %s %d %s", 1, unchk1, 2, unchk2); // expected-error {{local variable used in a checked scope must have a checked type}} expected-error {{parameter used in a checked scope must have a checked type or a bounds-safe interface}}
scanf("%d %s %d %s", &a, unchk1, &b, unchk2); // expected-error {{local variable used in a checked scope must have a checked type}} expected-error {{parameter used in a checked scope must have a checked type or a bounds-safe interface}}
MyScanf("%d %s %d %s", &a, unchk1, &b, unchk2); // expected-error {{local variable used in a checked scope must have a checked type}} expected-error {{parameter used in a checked scope must have a checked type or a bounds-safe interface}}


// For printf-like functions %s is allowed only with arg type _Nt_array_ptr or _Nt_checked.
printf("%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}
MyPrintf("%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}
scanf("%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}
MyScanf("%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}

// For scanf-like functions %s is disallowed in checked scope.
scanf("%s", arr); // expected-error {{in a checked scope %s format specifier is not allowed in scanf}}
MyScanf("%s", arr); // expected-error {{in a checked scope %s format specifier is not allowed in scanf}}

_Nt_array_ptr<char> p = "a";

// For printf-like functions %s is allowed in checked scope.
printf("%10s", p);
MyPrintf("%10s", p);

// For scanf-like functions %s is disallowed in checked scope.
scanf("%10s", p); // expected-error {{in a checked scope %10s format specifier is not allowed in scanf}} expected-error {{in a checked scope width is not allowed with format specifier in scanf}}
MyScanf("%10s", p); // expected-error {{in a checked scope %10s format specifier is not allowed in scanf}} expected-error {{in a checked scope width is not allowed with format specifier in scanf}}

_Ptr<int> i = 0;
_Ptr<void> q = 0;

// For printf/scanf-like functions %n is disallowed in checked scope.
printf("hello\n%n", i); // expected-error {{in a checked scope %n format specifier is not allowed}}
MyPrintf("hello\n%n", i); // expected-error {{in a checked scope %n format specifier is not allowed}}
scanf("%n", i); // expected-error {{in a checked scope %n format specifier is not allowed}}
MyScanf("%n", i); // expected-error {{in a checked scope %n format specifier is not allowed}}

// For printf-like functions %p is allowed in checked scope.
printf("%p", q);
MyPrintf("%p", q);

// For scanf-like functions %p is disallowed in checked scope.
scanf("%p", &q); // expected-error {{in a checked scope %p format specifier is not allowed with scanf}}
MyScanf("%p", &q); // expected-error {{in a checked scope %p format specifier is not allowed with scanf}}

int arr _Checked[5];

// For printf-like functions for format specifiers other than %s, only scalar
// arg types are allowed in checked scope.
printf("%d", arr[4]);
printf("%d", *i);

printf("%d", arr); // expected-error {{format specifies type 'int' but the argument has type '_Array_ptr<int>'}} expected-error {{in a checked scope %d format specifier requires scalar argument}}
MyPrintf("%d", arr); // expected-error {{format specifies type 'int' but the argument has type '_Array_ptr<int>'}} expected-error {{in a checked scope %d format specifier requires scalar argument}}

// For scanf-like functions only _Ptr arg types are allowed in checked scope.
scanf("%d", arr); // expected-error {{in a checked scope %d format specifier requires _Ptr argument}}
MyScanf("%d", arr); // expected-error {{in a checked scope %d format specifier requires _Ptr argument}}
}
}

void f2 (_Nt_array_ptr<char> p, _Array_ptr<char> arr, _Ptr<FILE> fp) {
char *s;

// In checked scope, fprintf, sprintf, snprintf, fscanf, sscanf, etc follow
// rules similar to printf/scanf.

_Checked {
fprintf(fp, "%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}
sprintf(s, "%s", arr); // expected-error {{local variable used in a checked scope must have a checked type}}
snprintf(p, 1, "%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}

fscanf(fp, "%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}
sscanf(p, "%s", arr); // expected-error {{in a checked scope %s format specifier requires null-terminated argument}}
fscanf(fp, "%s", arr); // expected-error {{in a checked scope %s format specifier is not allowed in scanf}}
sscanf(p, "%s", arr); // expected-error {{in a checked scope %s format specifier is not allowed in scanf}}
}
}

void f3(_Nt_array_ptr<char> p, _Array_ptr<char> arr, _Ptr<FILE> fp) {
va_list args;
char *s;

// In checked scope, vprintf, vscanf, etc follow rules similar to
// printf/scanf.

_Checked {
vprintf(p, args); // expected-error {{local variable used in a checked scope must have a checked type}}
vfprintf(fp, "%s", args); // expected-error {{local variable used in a checked scope must have a checked type}}
Expand All @@ -104,6 +178,9 @@ _Checked {
}

void f4 (_Nt_array_ptr<char> p, _Nt_array_ptr<wchar_t> w, _Ptr<int> ptr, _Ptr<void> voidPtr) {
// These diagnostics issued by the -Wformat family of flags are treated as
// errors in checked scope.

_Checked {
printf(p); // expected-error {{format string is not a string literal (potentially insecure)}}
MyPrintf(p); // expected-error {{format string is not a string literal (potentially insecure)}}
Expand Down Expand Up @@ -135,11 +212,6 @@ _Checked {
scanf("", p); // expected-error {{format string is empty}}
MyScanf("", p); // expected-error {{format string is empty}}

printf("%10s", p);
MyPrintf("%10s", p);
scanf("%10s", p); // expected-error {{in a checked scope width is not allowed with format specifier in scanf}}
MyScanf("%10s", p); // expected-error {{in a checked scope width is not allowed with format specifier in scanf}}

printf("%d", 1.0); // expected-error {{format specifies type 'int' but the argument has type 'double'}}
MyPrintf("%d", 1.0); // expected-error {{format specifies type 'int' but the argument has type 'double'}}
scanf("%d", 1.0); // expected-error {{format specifies type 'int *' but the argument has type 'double'}}
Expand Down Expand Up @@ -175,16 +247,21 @@ _Checked {
scanf("%P", p); // expected-error {{invalid conversion specifier 'P'}}
MyScanf("%P", p); // expected-error {{invalid conversion specifier 'P'}}

printf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}}
MyPrintf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}}
scanf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}}
MyScanf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}}
printf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}} expected-error {{in a checked scope %S format specifier requires scalar argument}}
MyPrintf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}} expected-error {{in a checked scope %S format specifier requires scalar argument}}
scanf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}} expected-error {{in a checked scope %S format specifier requires _Ptr argument}}
MyScanf("%S", w); // expected-error {{'S' conversion specifier is not supported by ISO C}} expected-error {{in a checked scope %S format specifier requires _Ptr argument}}

int i;
printf("%*d", 123456789, i);

printf("%*d"); // expected-error {{'*' specified field width is missing a matching 'int' argument}}
MyPrintf("%*d"); // expected-error {{'*' specified field width is missing a matching 'int' argument}}
scanf("%*d"); // Note: This is safe because * indicates the data is to be read from the stream but ignored (i.e. it is not stored in the location pointed by an argument).
MyScanf("%*d"); // Note: This is safe because * indicates the data is to be read from the stream but ignored (i.e. it is not stored in the location pointed by an argument).

printf("%.*d", 123456789, i);

printf("%.*d"); // expected-error {{'.*' specified field precision is missing a matching 'int' argument}}
MyPrintf("%.*d"); // expected-error {{'.*' specified field precision is missing a matching 'int' argument}}
scanf("%.*d"); // expected-error {{invalid conversion specifier '.'}}
Expand Down Expand Up @@ -224,6 +301,8 @@ _Checked {
int MyPrintf_NoFormatAttr(const char *format : itype(_Nt_array_ptr<const char>), ...);

void f5(_Nt_array_ptr<char> p) {
// A custom variadic function that does not have an __attribute__((format))
// is not allowed in checked scope.
_Checked {
MyPrintf_NoFormatAttr("%s", "abc"); // expected-error {{cannot use this variable arguments function in a checked scope or function}}
}
Expand Down