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

webpki with Ring 0.17 and untrusted 0.9 #43

Closed
JackTemaki opened this issue Apr 12, 2023 · 9 comments
Closed

webpki with Ring 0.17 and untrusted 0.9 #43

JackTemaki opened this issue Apr 12, 2023 · 9 comments

Comments

@JackTemaki
Copy link

JackTemaki commented Apr 12, 2023

I am currently trying to build rustls for the riscv64 architecture. Unfortunately riscv support is only possible with ring 0.17 and not with ring 0.16.20, and I tried to find my way through the dependencies and ended up here with getting build errors. The webpki build fails when having a mismatch in the untrusted versions (0.7.1 for webpki and 0.9 for ring), and also fails when switching to 0.9 for both (I assume the API changed). Are there plans yet to update the ring/unstrusted dependencies?

If not, I would try myself to fix this somehow.

@djc
Copy link
Member

djc commented Apr 12, 2023

On the rustls side, we have rustls/rustls#1108. We don't have a similar branch/PR for rustls-webpki yet, but if you want to submit one, that would be great!

@JackTemaki
Copy link
Author

Thanks for the quick reply!

On the rustls side, we have rustls/rustls#1108. We don't have a similar branch/PR for rustls-webpki yet, but if you want to submit one, that would be great!

Yes, I tried to build starting from that branch. I am not really proficient in Rust yet (only wrote some minor demo tools so far), but I am willing to learn it. So I will try and see what I can do!

@djc
Copy link
Member

djc commented Apr 12, 2023

Feel free to ask for help if/when you get stuck!

@JackTemaki
Copy link
Author

Unfortunately I got stuck quicker than I hoped. It is clear that compilation does not work anymore because of:
briansmith/untrusted@36f6dff
Where equality checks on Input were removed, probably because they used the as_slice_less_safe().Now it would be possible to use as_slice_less_safe() to check for equality in the code here instead, but then I guess there was a reason why this was removed, and simply adding the same code here is not the intended solution.

But I have no idea what would be a more secure way to check for equality/inequality here.

@JackTemaki
Copy link
Author

For now I used the straightforward way of replacing the checks just for sake of continuing with my build test, the branch for that is here:
main...JackTemaki:webpki:untrusted-0.9-riscv

@djc
Copy link
Member

djc commented Apr 18, 2023

I think the point is that in some places, we'd want constant-time comparison code (for example, using the constant_time_eq crate?) rather than a simple slice comparison (which can short-circuit and would then leak some information as a timing side channel). I'm honestly not sure that's a concern here. It is interesting that the referenced commit mentions this in the commit message:

I verified that ring and webpki can be changed to work with this change.

@JackTemaki
Copy link
Author

I mean the statement is not wrong, maybe it meant it can be changed to work by just adding the explicit as_slice_less_safe?. For locations where the length is known already (also to the outside) probably one can use the package you suggested.

So what are you proposing? Directly looking for constant time code or first merging the "naive" approach that behaves just like before? I can clean up the branch for a PR (also with the tests which are missing now), or play around with the package you suggested (or alternatives).

@djc
Copy link
Member

djc commented Apr 20, 2023

For each of the comparison site, we should make a judgement call whether constant-time comparison is necessary. The easy way out might be to do it for all of them, but that might be overkill.

Submitting your branch as a (draft?) PR is probably a good idea either way, will make it easier for us to review.

@ctz
Copy link
Member

ctz commented Oct 3, 2023

Addressed for this crate in #193

@ctz ctz closed this as completed Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants