Skip to content
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.

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

Proposed relaxing of cjalr sealing #86

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Proposed relaxing of cjalr sealing #86

merged 1 commit into from
Dec 2, 2024

Conversation

nwf
Copy link
Member

@nwf nwf commented Nov 13, 2024

Attempt at capturing #85

@nwf nwf force-pushed the 202411-relax_cjalr branch from 54497f9 to f3c2688 Compare November 13, 2024 13:55
Copy link
Collaborator

@rmn30 rmn30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to make a similar change in CJAL.

src/cheri_insts.sail Outdated Show resolved Hide resolved
@nwf nwf force-pushed the 202411-relax_cjalr branch from f3c2688 to 85e58b7 Compare November 13, 2024 14:09
@nwf nwf requested a review from rmn30 November 13, 2024 14:17
Copy link
Collaborator

@rmn30 rmn30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine except for comments. I may slightly prefer inverting the if so that the common cd==ra case is handled first but it does not matter.

src/cheri_insts.sail Outdated Show resolved Hide resolved
src/cheri_insts.sail Outdated Show resolved Hide resolved
@vmurali
Copy link
Collaborator

vmurali commented Nov 13, 2024

Lgtm. As Robert pointed out, can you fix the commentary in the JAL case

@nwf nwf force-pushed the 202411-relax_cjalr branch from 85e58b7 to 0dd27c1 Compare November 14, 2024 21:08
@nwf
Copy link
Member Author

nwf commented Nov 14, 2024

Copy-paste errors fixed, conditions flipped, and archdoc prose updated to explain the changes.

@vmurali
Copy link
Collaborator

vmurali commented Nov 14, 2024

Is there a security issue if cjal links an unsealed cap even for link register of cra? That means cjr cra should be relaxed to allow unsealed caps in cra. I don't see a problem with that though - if it's sealed, it has to be a backwards sentry, and if it's unsealed, I can jump anywhere with it.

@rmn30
Copy link
Collaborator

rmn30 commented Nov 15, 2024

Is there a security issue if cjal links an unsealed cap even for link register of cra? That means cjr cra should be relaxed to allow unsealed caps in cra. I don't see a problem with that though - if it's sealed, it has to be a backwards sentry, and if it's unsealed, I can jump anywhere with it.

I think it would be fine in that the interrupt disabling DoS attack that pushed us to add return sentries would still be mitigated (because you have to use cra as link when jumping to an interrupt disabling sentry), but it is a weakening of the "psuedo-CFI" we have now and isn't necessary to support non-standard link register as desired.

Specifically, most code will use cret to return and relaxing this to accept unsealed caps would allow an attacker more scope to substitute other capabilities in ROP attacks. This may not be a great risk given we have bounds checking and unforgeable capabilities but if we don't need to then why do it?

@vmurali
Copy link
Collaborator

vmurali commented Nov 15, 2024

Specifically, most code will use cret to return and relaxing this to accept unsealed caps would allow an attacker more scope to substitute other capabilities in ROP attack. This may not be a great risk given we have bounds checking and unforgeable capabilities

My rationale is that when cra is linked with cjal instead of cjalr, you are within your own code that you could have done the ROP anyway, hence not increasing the attack surface.

but if we don't need to then why do it?

Slight hardware simplification and in line with the design principle that if you want protection of the caller, use cjalr cra

@rmn30
Copy link
Collaborator

rmn30 commented Nov 19, 2024

My rationale is that when cra is linked with cjal instead of cjalr, you are within your own code that you could have done the ROP anyway, hence not increasing the attack surface.

But ROP is a code reuse attack which uses the victims own code against them. I agree that we need to articulate the threat model more clearly: it's difficult to imagine a scenario on CHERIoT where this would make a difference. I think it would be something like: the attacker finds a vulnerability that allows them to store a capability at an arbitrary offset in the victim's stack. They use this to overwrite the victim's return address with a capability of their choosing. If cret requires a return sentry this limits the attacker's choice of destination to those for which they can obtain a return sentry. This may seem far-fetched but attackers do tend to find a way if you let them, and I don't see a compelling reason to allow it.

Slight hardware simplification and in line with the design principle that if you want protection of the caller, use cjalr cra

I don't think it simplifies hardware to make return address generation different between cjal and cjalr. If they are the same then the hardware can be shared between them, though I defer to @kliuMsft .

@vmurali
Copy link
Collaborator

vmurali commented Nov 19, 2024

But ROP is a code reuse attack which uses the victims own code against them.

the attacker finds a vulnerability that allows them to store a capability at an arbitrary offset in the victim's stack. They use this to overwrite the victim's return address with a capability of their choosing. If cret requires a return sentry this limits the attacker's choice of destination to those for which they can obtain a return sentry.

If they are all in the same PCC, you are not gaining much.

The invariant I am maintaining is, the return is protected only if the call is. cjal creates an unprotected call (i.e. it can jump to anywhere in the code), so the return can also jump to anywhere in the code.

But I agree it's not necessary to weaken things.

archdoc/chap-cheri-riscv.tex Outdated Show resolved Hide resolved
archdoc/chap-cheri-riscv.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@rmn30 rmn30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Suggested clarifications in arch doc optional.

Attempt at capturing #85

Co-authored-by: Robert Norton <[email protected]>
@nwf nwf force-pushed the 202411-relax_cjalr branch from c9589ea to 2c456c7 Compare December 2, 2024 15:40
@nwf nwf merged commit 45454e2 into main Dec 2, 2024
2 checks passed
@nwf nwf deleted the 202411-relax_cjalr branch December 2, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (Ibex)
Development

Successfully merging this pull request may close these issues.

3 participants