capablevms / cheri-examples

CHERI sample C programs
Other
10 stars 13 forks source link

Add two negative examples #57

Closed 0152la closed 2 years ago

0152la commented 2 years ago

This is to start a conversation, if there are any additional vectors of attack to be considered. I have in mind one compartment attempting to attack another, or attempting to modify its own capabilities somehow. Also, one issue that isn't solved in the secure example is checking the number of expected compartments. I plan on submitting a separate PR adding the check in, and a negative example to exercise that.

Adds to initial negative examples, expected to fail to show that the security works. These contain:

ltratt commented 2 years ago

Maybe silly question but is it possible to have these examples use the existing code? At the moment it's a bit hard to read the PR because I think there's a lot of code duplication, so I'm not sure where to focus my attention. Warning: I might be wrong!

0152la commented 2 years ago

What I have done is mark the files changed (in relation to their counterparts in secure/) with a suffix. So compartments-redirect_clr.s is where changes are made compared to compartments.s (while main.c has no changes in secure-redirect_clr compared to secure/main.c). Thus, an option is doing a diff locally.

To help see things in the Github UI, I could remove this commit, force push one with just copies of the base secure files, and then run another commit with the changes over that?

Overall, yes, there is a ton of code duplication which I dislike, but it was the option I came up with to integrate stuff in our existing build workflow.

ltratt commented 2 years ago

I think it'll benefit all of us to have a solution where we can clearly see the important deltas rather than having lots of repetition. Can you have a look into how to reuse code between examples? I don't know the best way to do it, but it must be possible to do something, whether that's e.g. moving Makefiles up a level or whatever. Feel free to force push an update if that's the best way of doing thngs.

0152la commented 2 years ago

Well, in any case, by definition, these negative examples won't highlight the delta on the Github UI, as they should be new files we push in, so there still would be the need to do two commits as I've highlighted above. Changing the infrastructure would just remove the amount of code added within a commit.

I'll have another look at the common build mechanism [1] to see if I can do something.

[1] https://github.com/capablevms/cheri-examples/blob/master/build/Makefile.simple

ltratt commented 2 years ago

Thanks!

0152la commented 2 years ago

Ok, I've made a mess of everything, but for now, the differences should be emphasised in the latest commit [1], so this PR can proceed for now, at least in terms of correctness.

[1] https://github.com/capablevms/cheri-examples/pull/57/commits/6ca47879dad256f32220d2fc4d1f734c817a761a

ltratt commented 2 years ago

@jacobbramley Grateful for your thoughts on these examples!

0152la commented 2 years ago

The following is for buildbot testing

0152la commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build succeeded:

ltratt commented 2 years ago

Broadly speaking this looks OK to me, but the details do elude me somewhat!

@jacobbramley can you do another check? If you're happy with it, I think we should get this merged, and then ask @0152la to write something high-level so that I have a better chance of understanding what's going on!

0152la commented 2 years ago

Just note that I have still to work out the building issues; please do look at the actual code otherwise and see if there can be improvements done, or clarifying comments added.

0152la commented 2 years ago

@jacobbramley The build system should have been fixed now, and run-main should work. I think the Makefiles as they are could be moved on level up and shared, but it's better to leave that for the next change. Do let me know if there are any further infrastructure issues, and your thoughts on the essential changes to the security of the switcher.

ltratt commented 2 years ago

Once @jacobbramley is happy with this, you should squash.

ltratt commented 2 years ago

@0152la Let's get this squashed and merged. I think we can improve this, probably, but I think now is the right time to do a slightly more radical rethink about how we structure things.

0152la commented 2 years ago

Squashed.

ltratt commented 2 years ago

bors r+

(If @jacobbramley has additional comments, we can address those in a subsequent PR.)

bors[bot] commented 2 years ago

Build failed:

0152la commented 2 years ago

This last commit should fix it; I can try until it works, if you want.

ltratt commented 2 years ago

Good idea to try until it works.

0152la commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build failed:

0152la commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build succeeded:

ltratt commented 2 years ago

Please squash.

0152la commented 2 years ago

Squashed. For reference, the issue was that infrastructure changes affected previous examples, which I forgot to update.

ltratt commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build succeeded: