[randstruct] Automatically randomize a structure of function pointers

[void]

Summary

Strutures of function pointers are a good surface area for attacks. We
should therefore randomize them unless explicitly told not to.

[xbolva00]

clang/lib/Sema/SemaDecl.cpp

18070

Is this allowed by C standard?

[void]

clang/lib/Sema/SemaDecl.cpp

18070

Randomizing the layout of a structure of pointers or just randomizing a structure in general?

There is a major restriction to randomizing structures. The programmer must use designated initializers when initializing rather than relying upon a default ordering.

[MaskRay]

The main user of the feature is the Linux kernel. Does it still work with the change? If so, LG.

If not, some fixes may be needed which may possibly postpone the deployment of this Clang feature?

Last, it'd be good to know whether the GCC plugin randomize_layout_plugin.c does this similar thing.

[aaron.ballman]

While I agree with the security aspects of this in principle, it is not a conforming behavior in C and it runs significant risk of breaking existing code such that it introduces new security issues. This behavior needs to be opt-in (either through the existing attribute or through a feature flag of some kind).

It also points out that we really, really should have a diagnostic for structure initialization using positional initialization instead of designated initialization if the structure is randomized. (I think that case should be an error, perhaps with an exception if the initialization is empty {}.)

[xbolva00]

While I agree with the security aspects of this in principle, it is not a conforming behavior in C and it runs significant risk of breaking existing code such that it introduces new security issues.

I agree strongly. This could happily can do more harm than good ("the road to hell is paved with good intentions"). Please leave it opt-in or introduce "modes" for this feature like -frandomize-struct=func-pointers | -frandomize-struct=xxx | -frandomize-struct=all.

[void]

In D123544#3445425, @xbolva00 wrote:

While I agree with the security aspects of this in principle, it is not a conforming behavior in C and it runs significant risk of breaking existing code such that it introduces new security issues.

I agree strongly. This could happily can do more harm than good ("the road to hell is paved with good intentions"). Please leave it opt-in or introduce "modes" for this feature like -frandomize-struct=func-pointers | -frandomize-struct=xxx | -frandomize-struct=all.

[Some context for this patch: the GCC plugin does this already. Our initial implementation of randstruct skipped that part.]

Could you explain a bit more why it's not considered conforming behavior in C? The entire feature is definitely unusual and introduces some caveats (the structure initialization being just one of them).

Here's an article on this feature and why randomizing structs of function pointers is Considered Good(tm): https://lwn.net/Articles/722293/

I'm okay with a flag, but will need to iterate on the name and such. There is the way to opt-out via the no_randomize_layout attribute. But that means this feature is on by default, which I think is where you concerns lie...

[MaskRay]

The Linux kernel already uses some options which deviate from standard C: __attribute__((__gnu_inline__)), -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang, -fno-delete-null-pointer-checks

If the feature is to emulate GCC_PLUGIN_STRUCTLEAK in kernel security/Kconfig.hardening and the GCC plugin already does something similar, I am fine with this patch.
(My main concern is the lack of testing from the base patch.)

[lebedev.ri]

Does this not lead to non-deterministic/non-reproducible builds?
I do not understand why this feature must be inflicted onto everyone.

[MaskRay]

In D123544#3446285, @lebedev.ri wrote:

Does this not lead to non-deterministic/non-reproducible builds?
I do not understand why this feature must be inflicted onto everyone.

It seems that the feature fixes the randomization seed in the option -frandomize-layout-seed=, so it is deterministic. That said, it currently has a possible Windows vs non-Windows host difference, probably related to MSVC's std::mt19937.
My concern is related to the lack of -frandomize-layout-seed-file= testing in the base patch.

[void]

In D123544#3446285, @lebedev.ri wrote:

Does this not lead to non-deterministic/non-reproducible builds?

It's deterministic based on the randomization seed.

I do not understand why this feature must be inflicted onto everyone.

A developer must mark structures with the randomize_layout attribute and supply a randomization seed for the randstruct feature to be enabled. It's only when a randomization seed is supplied that it'll randomize structures consisting of only function pointers (which happens a lot in Linux). And you're able to opt-out of such a thing.

[aaron.ballman]

In D123544#3446265, @void wrote:

Could you explain a bit more why it's not considered conforming behavior in C? The entire feature is definitely unusual and introduces some caveats (the structure initialization being just one of them).

Given:

typedef void (*func_ptr)(void);

struct S {
  func_ptr f1, f2, f3;
};

void func(void);

struct S s = {func, 0, func};

C requires that s.f1 and s.f3 point to func and s.f2 is a null pointer, but if you automatically randomize the layout of that structure as in this patch, this strictly conforming code will break.

However, I had forgotten that the base feature *requires* the user to pass a randomization seed via a flag in addition to requiring the attribute (thank you for bringing that back to my attention). Because this feature requires a feature flag to enable it, this behavior *is* a conforming extension (the user has to take an action to get the new behavior). That said, I'm still not convinced we want to do this automagically for users -- it's *really* easy for that flag to be set in a makefile somewhere and the user has no idea that their (non-designated) initialization is now a security vulnerability. If we had error diagnostics when the user is about to shoot their foot off, I'd be more comfortable with the automatic hardening behavior.

[xbolva00]

Because this feature requires a feature flag to enable it, this behavior *is* a conforming extension (the user has to take an action to get the new behavior).

Well, then we should have proper C test (ok if linux only or so) to show case behaviour of this patch with and without that feature flag.

Looking at this unit test, I thought that would get this feature with simple "clang source.c" invocation.

[xbolva00]

If we had error diagnostics when the user is about to shoot their foot off, I'd be more comfortable with the automatic hardening behavior.

This feature IMHO should be enabled with simple easy to understand flag, not silently and automatically do something which may break code or create surprises in debuggers.

[void]

In D123544#3446416, @aaron.ballman wrote:

However, I had forgotten that the base feature *requires* the user to pass a randomization seed via a flag in addition to requiring the attribute (thank you for bringing that back to my attention). Because this feature requires a feature flag to enable it, this behavior *is* a conforming extension (the user has to take an action to get the new behavior). That said, I'm still not convinced we want to do this automagically for users -- it's *really* easy for that flag to be set in a makefile somewhere and the user has no idea that their (non-designated) initialization is now a security vulnerability. If we had error diagnostics when the user is about to shoot their foot off, I'd be more comfortable with the automatic hardening behavior.

We should definitely emit an error if a user is trying to use a default initializer for a structure that's up for randomization. It's something that affects the whole feature, not just structs of function pointers. Let me work on that. But otherwise are you okay with this patch?

[void]

In D123544#3446519, @xbolva00 wrote:

If we had error diagnostics when the user is about to shoot their foot off, I'd be more comfortable with the automatic hardening behavior.

This feature IMHO should be enabled with simple easy to understand flag, not silently and automatically do something which may break code or create surprises in debuggers.

A developer has to go out of their way to indicate that they want to use randstruct. The attribute has no effect unless they take an extra action to enable it (i.e. passing one of the seed flags). I agree with Aaron though that we need to error out if the developer uses default initialization for a randomized structure.

[xbolva00]

passing one of the seed flags

Yeah, then this is fine.

[nickdesaulniers]

if the developer uses default initialization for a randomized structure.

Yeah, I suspect that @aaron.ballman @lebedev.ri @xbolva00 missed that this just extends the newly added opt-in -frandomize-layout-seed= (https://reviews.llvm.org/D121556). Why a developer would want the latter but not the former, when the existing GCC plugin being used by the kernel doesn't differentiate is somewhat a tangential feature request that doesn't need to be two distinct things at the moment for existing consumers.

We should definitely emit an error if a user is trying to use a default initializer for a structure that's up for randomization. It's something that affects the whole feature, not just structs of function pointers.

Agree though. Perhaps land that first, then revisit this?

[aaron.ballman]

In D123544#3446814, @void wrote:

In D123544#3446416, @aaron.ballman wrote:

However, I had forgotten that the base feature *requires* the user to pass a randomization seed via a flag in addition to requiring the attribute (thank you for bringing that back to my attention). Because this feature requires a feature flag to enable it, this behavior *is* a conforming extension (the user has to take an action to get the new behavior). That said, I'm still not convinced we want to do this automagically for users -- it's *really* easy for that flag to be set in a makefile somewhere and the user has no idea that their (non-designated) initialization is now a security vulnerability. If we had error diagnostics when the user is about to shoot their foot off, I'd be more comfortable with the automatic hardening behavior.

We should definitely emit an error if a user is trying to use a default initializer for a structure that's up for randomization. It's something that affects the whole feature, not just structs of function pointers.

Agreed.

Let me work on that. But otherwise are you okay with this patch?

Yes, I'm okay with the direction of this patch. However, because this still automatically randomizes some structure layouts once the user passes the flag, I think the diagnostic above should perhaps land first unless there's some strong reason not to. (To be honest, I probably should have insisted on it in the first patch, but this one feels significantly more dangerous because it involves function pointers specifically and is more of an "automatic" feature than the first patch.)

[void]

In D123544#3448046, @aaron.ballman wrote:

In D123544#3446814, @void wrote:

Let me work on that. But otherwise are you okay with this patch?

Yes, I'm okay with the direction of this patch. However, because this still automatically randomizes some structure layouts once the user passes the flag, I think the diagnostic above should perhaps land first unless there's some strong reason not to. (To be honest, I probably should have insisted on it in the first patch, but this one feels significantly more dangerous because it involves function pointers specifically and is more of an "automatic" feature than the first patch.)

It was definitely an oversight on my part as well. This patch will be going in llvm 15, not 14, so there's probably not a huge rush to get this in first. I'll focus on the error and have a patch out soon-ish.

[kees]

I tested this (with D123958), and it appears to be working as intended. These two fptrs are the first and second listed, and are happily randomized:

[    0.000000] LSM: offsetof(struct security_hook_heads, ptrace_access_check): 816
[    0.000000] LSM: offsetof(struct security_hook_heads, ptrace_traceme): 512

[void]

Rebase with top-of-tree.

PTAL.

[MaskRay]

Happy if Aaron is happy.

[aaron.ballman]

LGTM

clang/lib/Sema/SemaDecl.cpp

if (Record) {
     }
 
     if (!Completed)
       Record->completeDefinition();
 
     // Handle attributes before checking the layout.
     ProcessDeclAttributeList(S, Record, Attrs);
 
-    // Maybe randomize the record's decls.
-    if (!getLangOpts().CPlusPlus && Record->hasAttr<RandomizeLayoutAttr>() &&
+    // Check to see if a FieldDecl is a pointer to a function.
+    auto IsFunctionPointer = [&](const Decl *D) {
+      const FieldDecl *FD = dyn_cast<FieldDecl>(D);
+      if (!FD)
+        return false;
+      QualType FieldType = FD->getType().getDesugaredType(Context);
+      if (isa<PointerType>(FieldType)) {
+        QualType PointeeType = cast<PointerType>(FieldType)->getPointeeType();
+        return PointeeType.getDesugaredType(Context)->isFunctionType();
+      }
+      return false;
+    };
+
+    // Maybe randomize the record's decls. We automatically randomize a record
+    // of function pointers, unless it has the "no_randomize_layout" attribute.
+    if (!getLangOpts().CPlusPlus &&
+        (Record->hasAttr<RandomizeLayoutAttr>() ||
+         (!Record->hasAttr<NoRandomizeLayoutAttr>() &&
+          llvm::all_of(Record->decls(), IsFunctionPointer))) &&
         !Record->isUnion() && !getLangOpts().RandstructSeed.empty() &&
         !Record->isRandomized()) {
       SmallVector<Decl *, 32> NewDeclOrdering;
       if (randstruct::randomizeStructureLayout(Context, Record,
                                                NewDeclOrdering))
         Record->reorderDecls(NewDeclOrdering);
     }
 

clang/unittests/AST/RandstructTest.cpp

TEST(RANDSTRUCT_TEST, AnonymousStructsAndUnionsReferenced) {
 
   const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
   long OriginalDeclCount = declCount(RD);
 
   EXPECT_TRUE(RD->isRandomized());
   EXPECT_EQ(OriginalDeclCount, declCount(RD));
 }
 
+TEST(RANDSTRUCT_TEST, AutoRandomizeStructOfFunctionPointers) {
+  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
+    typedef void (*func_ptr)();
+
+    struct test {
+      func_ptr a;
+      func_ptr b;
+      func_ptr c;
+      func_ptr d;
+      func_ptr e;
+      func_ptr f;
+      func_ptr g;
+    };
+  )c");
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+
+  EXPECT_TRUE(RD->isRandomized());
+}
+
+TEST(RANDSTRUCT_TEST, DisableAutoRandomizeStructOfFunctionPointers) {
+  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
+    typedef void (*func_ptr)();
+
+    struct test {
+      func_ptr a;
+      func_ptr b;
+      func_ptr c;
+      func_ptr d;
+      func_ptr e;
+      func_ptr f;
+      func_ptr g;
+    } __attribute__((no_randomize_layout));
+  )c");
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+
+  EXPECT_FALSE(RD->isRandomized());
+}
+
 } // namespace ast_matchers
 } // namespace clang