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

Change trn->trn to box to fix a soundness hole #3591

Merged
merged 3 commits into from
Jul 15, 2020

Conversation

jasoncarr0
Copy link
Contributor

@jasoncarr0 jasoncarr0 commented Jul 14, 2020

Currently, this causes some compile errors in collections/persistent/_map_node
And they really don't seem trivial to fix; it's relying pretty heavily on the ability to mutate some trn views of trns. It can't be changed to ref either since ref also goes to box.

So I'm not sure we can make this change without going forward with extracting/non-extracting

Fixes #3572

@jemc
Copy link
Member

jemc commented Jul 14, 2020

Thanks. Let's continue the discussion in #3572

We will also discuss in the sync call today, I expect.

@jemc
Copy link
Member

jemc commented Jul 14, 2020

#3592 should resolve the issue with this - so after that's merged this can be rebased, after which it should pass tests and be mergable.

@SeanTAllen
Copy link
Member

#3592 has been merged.

@jasoncarr0 jasoncarr0 marked this pull request as ready for review July 15, 2020 01:05
@jasoncarr0
Copy link
Contributor Author

Huh, that was a lot easier then it looked. I ran into a lot of trouble with using iso. But then again I didn't have asynchronous compilation for that file.

@SeanTAllen
Copy link
Member

@Theodus can you do the release notes for this and add to release issue?

#3563

@SeanTAllen
Copy link
Member

@jasoncarr0 I'm going to squash this when it's merged. Is there a single commit comment that you think would be appropriate for this? I'd prefer if you craft something as it is your work, but I can craft one if need be.

@SeanTAllen SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Jul 15, 2020
@jasoncarr0
Copy link
Contributor Author

I don't think I'm too attached to the particular commit message, maybe something like:
"Change trn->trn to box to fix a soundness hole"

@SeanTAllen SeanTAllen changed the title Weaken trn->trn to be box Change trn->trn to box to fix a soundness hole Jul 15, 2020
@SeanTAllen
Copy link
Member

Comment for when squashing:

Change trn->trn to box to fix a soundness hole

As discovered in #3571, it is currently possible to violate capability guarantees in the current viewpoint adaptation table, but this could be prevented by updating ponyc to use George Steed's viewpoint adaptation rules which distinguish between extracting and non-extracting viewpoint adaptation - (#2815). This commit patches the immediate soundness hole without needing to complete the larger chunk of work.

@sylvanc pointed out that we could easily close the latter hole without implementing the extensive changes required by George Steed's model by simply updating our single viewpoint adaptation table.

This fix will break existing code, in that it might remove some valid/sound uses of non-extracting capabilities to fix the soundness hole for extracting capabilities. But it fixes the hole and prevent the exploit.

To be totally clear, fixing this issue in this fashion means we updated the trn row of our viewpoint adaptation table as seen in the tutorial at https://tutorial.ponylang.io/reference-capabilities/combining-capabilities.html to the following:


iso field trn field ref field val field box field tag field
trn origin iso box box val box tag

In other words, to close the hole, this commit makes it so that the viewpoint adaptation trn->trn would now be a box rather than a trn.

Note that in George Steed's model, the non-extracting form is trn and the extracting form is val, so you would expect that we can choose val for the "weaker" one but we actually can't guarantee val until we know we're extracting, which we don't know because we haven't made that distinction. So the best we can do is have it be box.

Fixes #3571

@SeanTAllen
Copy link
Member

@jasoncarr0 this needs to be rebased against master to pick up @Theodus' changes to the persistent collection, do you know how to do update your fork with the latest changes from ponyc master and then how to rebase your branch from those changes?

@jasoncarr0
Copy link
Contributor Author

Whoops, yup I do know how to do that, didn't get enough sleep today :P

@SeanTAllen
Copy link
Member

@jasoncarr0 i think you accidentally closed this PR.

@jasoncarr0 jasoncarr0 reopened this Jul 15, 2020
@jasoncarr0
Copy link
Contributor Author

Yeah apparently I don't know how to rebase. Somehow it didn't pick up my commits, so Github just decided it was merged.

@SeanTAllen
Copy link
Member

@jasoncarr0 two options, i can walk you through the process or... you can delete your fork and start from a new fork that will get you the latest ponyc master and you can open a PR from that. Which would you prefer to try?

@SeanTAllen
Copy link
Member

@jemc you are ok with this being merged yes?

Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

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

Yep, good to merge once CI passes.

Thanks, @jasoncarr0!

@SeanTAllen SeanTAllen merged commit a576793 into ponylang:master Jul 15, 2020
github-actions bot pushed a commit that referenced this pull request Jul 15, 2020
@SeanTAllen SeanTAllen mentioned this pull request Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soundness hole: Update viewpoint adaptation table
3 participants