Page MenuHomePhabricator

[attributes] Add a facility for defining and enforcing a Trusted Computing Base.
ClosedPublic

Authored by NoQ on Nov 20 2020, 3:45 PM.

Details

Summary

Patch by Sean Dooher! I'll be addressing the review comments.

The point is to markup a section of code (a set of functions) that should be isolated for security, basically like a TCB. Such section of code, being privileged in some specific manner, would not be allowed to exercise arbitrary behavior, so calling a function that's outside the set from a function that's inside the set is not allowed; they can only call each other. This is ultimately supposed to achieve security of the system with respect to that privilege through audit of the TCB.

The patch adds an attribute enforce_tcb to define a TCB and a warning -Wtcb-enforcement for violating the enforcement. Additionally it adds an attribute enforce_tcb_leaf that allows opting out of enforcement for individual harmless functions: such "leaf" functions are allowed to be called from the respective TCB but aren't forced into the TCB themselves.

Diff Detail

Event Timeline

NoQ created this revision.Nov 20 2020, 3:45 PM
NoQ requested review of this revision.Nov 20 2020, 3:45 PM

LGTM! But I'd like other folks to take a look at it first.

This feels an awful lot like a set of attributes we already have -- can capability attributes be used for this instead? https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#basic-concepts-capabilities The documentation is largely centered around thread safety, but the gist of the idea seems similar to what you're proposing here -- certain parts of the program have different roles and you want to control how functions in one role can interact with functions in another role.

(I'll give a more thorough review to the proposed patch here but I wanted to pose that question first as it may obviate the need for this patch.)

Thread safety attributes want callers of a function to have the same attribute, while this change wants callees to have the same attribute. So the attributes propagate in different directions.

By contraposition the absence of an attribute propagates the other way around as the attribute itself, so you could have a role "untrusted", and callers of untrusted functions would have to be untrusted as well.

I guess it depends on how many functions need to be annotated one way or the other, if the TCB-based functions are a small subset of the code then this attribute is better, if most functions are based on the TCB and only some are not, the capability-based approach would be better.

NoQ added a comment.Nov 23 2020, 11:55 PM

if the TCB-based functions are a small subset of the code then this attribute is better

Yes, that's exactly the case. We want most functions to be untrusted by default but also have no restrictions imposed or warnings emitted for them when they do their usual function things.

In D91898#2413004, @NoQ wrote:

if the TCB-based functions are a small subset of the code then this attribute is better

Yes, that's exactly the case. We want most functions to be untrusted by default but also have no restrictions imposed or warnings emitted for them when they do their usual function things.

Excellent, thank you both for considering it!

clang/include/clang/Basic/Attr.td
3658

I don't think GCC supports these attributes, so this spelling should probably be Clang rather than GCC.

clang/include/clang/Basic/AttrDocs.td
5717

Are there plans to support this on function pointers or other kinds of callable things?

Also, it seems rather odd that C++ methods are not checked. I could somewhat understand not checking virtual functions, but why not member functions where the target is known? Why not static member functions?

clang/include/clang/Basic/DiagnosticGroups.td
1246 ↗(On Diff #306791)

Are you planning on adding more diagnostics under this group? If not, I'd suggest defining it inline (see below).

clang/include/clang/Basic/DiagnosticSemaKinds.td
11122
11123

...assuming the group won't be reused by too many diagnostics.

clang/include/clang/Sema/Sema.h
12433

Can you make these const pointers?

clang/lib/Sema/SemaChecking.cpp
16071
16073
16074–16076
16080–16085

Pretty sure you can remove the manual loops here with something like this.

16087

Can you be sure to run the patch through clang-format, this looks like it's over the 80 col limit.

16089
16091–16093
clang/lib/Sema/SemaDeclAttr.cpp
7520
7525
clang/test/Sema/attr-enforce-tcb.c
2
16

Some more interesting tests:

// Ensure that attribute merging works as expected across redeclarations.
void foo10() PLACE_IN_TCB("baz");
void foo10() PLACE_IN_TCB("bar");
void foo10() PLACE_IN_TCB("quux");

// Ensure that attribute instantiation works as expected.
template <typename Ty>
void foo11(Ty) PLACE_IN_TCB("bar");

// Basic checks
void foo12(void) __attribute__((enforce_tcb(12))); // wrong arg type
[[clang::enforce_tcb("oops")]] int foo13; // Wrong subject type
void foo14(void) __attribute__((enforce_tcb)); // Wrong number of arguments
void foo15(void) __attribute__((enforce_tcb("test", 12)); // Wrong number of arguments
// Add similar basic checks for enforce_tcb_leaf.
NoQ marked 16 inline comments as done.Dec 1 2020, 1:41 AM
NoQ added inline comments.
clang/include/clang/Basic/AttrDocs.td
5717

Yup, sounds like there are plans for function pointers. We're likely to have a follow-up patch for that. I'll relax the statement to "currently".

With C++ we basically didn't make up our mind yet with respect to what we want. Like, it's kind of strange to isolate individual members of the class into a TCB, it sounds like it'd make a lot more sense on per-class basis but we'll have to see. Also as a matter of fact, the current implementation does check (and emit warnings for) C++ methods. I'll add a test and a TODO to figure out what exactly do we want and drop that part of the documentation.

clang/include/clang/Basic/DiagnosticGroups.td
1246 ↗(On Diff #306791)

Aha, ok, not yet but i guess we can always bring the group back if we need more diagnostics.

clang/lib/Sema/SemaChecking.cpp
16080–16085

std::inserter doesn't seem to work with llvm::StringSet but llvm::for_each works and seems to be more compact(?)

16087

Umm yeah indeed thanks!

16091–16093

TIL that << Callee adds quotes automatically. I should use clang diagnostic builders more often :)

NoQ updated this revision to Diff 308581.Dec 1 2020, 1:41 AM
NoQ marked 5 inline comments as done.

Fxd!

dexonsmith resigned from this revision.Dec 3 2020, 5:27 PM
aaron.ballman added inline comments.Dec 10 2020, 9:57 AM
clang/include/clang/Basic/Attr.td
3666

Are these two attributes mutually exclusive, or would it make sense for a function to use both of these at the same time?

clang/lib/Sema/SemaChecking.cpp
16080–16085

Weird about std::inserter but this is a good improvement as-is.

16091–16093

Yeah -- the diagnostic engine knows how to format named things and will automatically insert quotes for you when given a named thing. Super handy, but not always super obvious.

NoQ updated this revision to Diff 313633.Dec 23 2020, 5:47 PM

Add an error for conflicting attributes!

NoQ added inline comments.Dec 23 2020, 5:54 PM
clang/include/clang/Basic/Attr.td
3666

Indeed! They should be mutually exclusive for the same TCB identifier but the same function can still be a non-leaf member of one TCB and a leaf member of another TCB.

Added an error and a test.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11121

Do i understand correctly that while "unknown attribute" is a warning ("must be an attribute for some other compiler, let's ignore"), misuse of a known attribute is typically an error ("ok, whatever you meant here, i have an opinion about what this really means and i don't like it")?

clang/test/Sema/attr-enforce-tcb-errors.cpp
24

Unfortunately the new facility doesn't catch this case because in handleEnforceTCBAttr() the function doesn't yet know that it's a redeclaration. I think this case is more important to catch than the straightforward case (because it's very easy to make this mistake), so i'll try to find a better place to emit the error. Is it ok if this goes into a follow-up patch?

aaron.ballman added inline comments.Jan 4 2021, 10:42 AM
clang/include/clang/Basic/AttrDocs.td
5734
5735
clang/include/clang/Basic/DiagnosticSemaKinds.td
11121

It depends strongly on the attribute and the problem at hand. My usual rule of thumb is: if the attribute is being ignored because the code makes no sense, it's an error, but if the attribute can still be useful (perhaps after some adjustment), then warn. e.g., putting a calling convention attribute on a variable of type int makes no sense -- that should probably err rather than warn and ignore because the user did something baffling.

I could go either way on this one. Giving an error and forcing the user to say what they mean is appealing, but it would also be defensible to warn about the conflicting attribute and ignore just that one (retaining the original attribute). I would say leave it as an error and we can downgrade to a warning later if there's feedback from compelling real world use cases.

clang/test/Sema/attr-enforce-tcb-errors.cpp
24

I was about to comment on the code in SemaDeclAttr.cpp but skipped down here to see if you had a redeclaration test first, and found this comment instead. :-D I think you need to follow the mergeFooAttr pattern to enforce this properly (and I think it probably should be done in the initial patch, but it shouldn't be too painful hopefully). The basic idea is that you change mergeDeclAttribute() in SemaDecl.cpp to call S.mergeEnforceTCBAttr() that checks whether there are conflicting attributes or not. Then in handleEnforceTCBAttr() in SemaDeclAttr.cpp, you can call mergeEnforceTCBAttr() to handle the straightforward case.

NoQ updated this revision to Diff 314846.Jan 6 2021, 3:44 AM

Handle conflicts across redeclarations! Fix typos and documentation formatting.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11121

retaining the original attribute

There's an interesting aspect of error recovery here (which i didn't address yet but added FIXMEs). We probably shouldn't emit additional warnings based on the attributes about which we've already reported a conflict error; that's just unnecessary clutter. That'd mean that we should always discard the non-leaf attribute and keep the leaf attribute (if we can at all discard attributes). Or keep both and repeat the conflict check during checking in order to suppress the warning.

clang/test/Sema/attr-enforce-tcb-errors.cpp
24

Found it, thanks!!

NoQ marked 5 inline comments as done.Jan 6 2021, 3:45 AM
aaron.ballman added inline comments.Jan 6 2021, 6:28 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11121

That'd mean that we should always discard the non-leaf attribute and keep the leaf attribute (if we can at all discard attributes). Or keep both and repeat the conflict check during checking in order to suppress the warning.

You can do TheDecl->dropAttr<EnforceTCB>(); to drop the non-leaf attributes (or, if easy, it's better to not add the problematic attribute in the first place). Would you like to do that work in this patch or as a follow-up? (I'm fine either way.)

clang/lib/Sema/SemaChecking.cpp
16089

Can you rename this to be something other than a type name?

NoQ updated this revision to Diff 314898.Jan 6 2021, 7:53 AM
NoQ marked 2 inline comments as done.

Drop broken attributes. Fix naming.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11121

Hmm, that's indeed an easy and reasonable solution. Ideally i should only drop attributes with the same argument but that's too far into the area of diminishing returns for me :)

aaron.ballman accepted this revision.Jan 8 2021, 5:48 AM

LGTM, thank you for this effort!

clang/include/clang/Basic/DiagnosticSemaKinds.td
11121

Yeah, I don't think we need to go that far unless there's some real world use cases demonstrating the need.

This revision is now accepted and ready to land.Jan 8 2021, 5:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 6:40 AM

Hi, this broke check-clang everywhere as far as I can tell. Please run tests before committing. I've reverted this for now in 419ef38a50293c58078f830517f5e305068dbee6.

For some host compilers, it also broke the build: http://lab.llvm.org:8011/#/builders/98/builds/3387/steps/9/logs/stdio (But that's impossible to know in advance.)

NoQ added a comment.Jan 11 2021, 7:18 AM

Yup, i'm sorry. I even saw this failure locally but was like "this test doesn't look familiar, probably someone else broke it" which i wouldn't have said if i looked at it for more than 1 second as it's strikingly obvious. My bad.