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

Propose side-channel resistant feature #153

Closed
wants to merge 2 commits into from

Conversation

dingelish
Copy link

Hi Marshall,

Thanks for your great work and I really enjoy using your base64 crate :-) It's really fast!

We know that rustls has been widely adopted and it depends on base64 to parse PEM files. Recently Apache httpd is constructing a new mod_tls based on rustls to replace the previous mod_ssl based on openssl. Glad to see rustls as well as base64 will be shipped with Apache httpd as well.

Recently I received a report from a group of security researchers (florian.sieck@uni-luebeck.de, s.berndt@uni-luebeck.de, thomas.eisenbarth@uni-luebeck.de), saying that this base64 decoder is not constant time thus suffer from side-channel attacks. The consequence is that PEM decoder and rustls suffers from side-channel attacks. Google has a good solution in BoringSSL: https://github.com/google/boringssl/blob/master/crypto/base64/base64.c . This proposed decoding algorithm is the researcher's work. They proved it in a unpublished paper. If you're interested, I can loop you in the mailing thread.

Now the solution is feature gated by slow_but_safe feature. cargo test passes. cargo bench shows performance slowdown. But it's acceptable if we only use this feature for PEM decoding (usually not a frequent operation). If this PR got merged, I'll create another PR for rustls.

Please feel free to edit/comment. Thanks!

Best, Yu

@marshallpierce
Copy link
Owner

Thanks for this; a constant time implementation has been on my mind for quite some time! However, rather than a feature approach, I'd prefer to go with making the "codec" part of base64 a generic type parameter so that the user could choose the current scalar implementation, your constant time scalar impl, and eventually AVX2 and NEON implementations. Most of the work would be in arranging the test corpus for the low-level encode and decode functions to be easily run for all implementations, and sprinkling the type parameter in all the right places. I think it would make sense to make the default for the codec be the current scalar impl, though I don't yet have a good sense of whether it would be a better API to pass around a struct for the codec (codec methods use &self) or just a type (no &self).

Would you have time to do that work?

@dingelish
Copy link
Author

@marshallpierce
thanks for the quick reply! i'm willing to do it! and i think i'd be better to contribute after you have the generic type added. i'll definitely get this PR rebased then :-) thanks!

@marshallpierce
Copy link
Owner

I'll have a draft PR up soon on trait-ifying the encoding and decoding, but in the mean time, can you confirm if the constant time decoding algorithm can have the same logic for the various DecodeError variants as the current way? If not, perhaps we should have different decoder implementations use different error types.

@marshallpierce
Copy link
Owner

As an example of the above, will it do the same "bogus trailing bits" detection when decoding? If not, that would mean that it would need a different config type as well.

@dingelish
Copy link
Author

@marshallpierce thank you! i'd lookinto the "bogus trailing bits detection" part this weekend!

@dingelish
Copy link
Author

i just quickly look into the current trailing detection part. i think the proposed solution in this PR shall share the same bogus trailing bits detection with the current decoder :-)

@marshallpierce
Copy link
Owner

Is the check for trailing bits insufficiently constant time, though, I wonder?

I'm making good progress on making pluggable "engines" so I should have a draft PR up for you to look at shortly to make sure it will fit with this proposed implementation.

@dingelish
Copy link
Author

@marshallpierce i kinda believe your current 'trailing bit detection' is almost identical to boringSSL's implemention:

https://boringssl.googlesource.com/boringssl/+/refs/heads/master/crypto/base64/base64.c#393

how do you think?

btw: i sent my PR to the researchers from University of Lübeck and they reviewed it and approved :-)

@dingelish dingelish closed this Feb 14, 2021
crapStone added a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [base64](https://github.com/marshallpierce/rust-base64) | dependencies | minor | `0.13.1` -> `0.20.0` |

---

### Release Notes

<details>
<summary>marshallpierce/rust-base64</summary>

### [`v0.20.0`](https://github.com/marshallpierce/rust-base64/blob/HEAD/RELEASE-NOTES.md#&#8203;0200)

[Compare Source](marshallpierce/rust-base64@v0.13.1...v0.20.0)

##### Breaking changes

-   Update MSRV to 1.57.0
-   Decoding can now either ignore padding, require correct padding, or require no padding. The default is to require correct padding.
    -   The `NO_PAD` config now requires that padding be absent when decoding.

#### 0.20.0-alpha.1

##### Breaking changes

-   Extended the `Config` concept into the `Engine` abstraction, allowing the user to pick different encoding / decoding implementations.
    -   What was formerly the only algorithm is now the `FastPortable` engine, so named because it's portable (works on any CPU) and relatively fast.
    -   This opens the door to a portable constant-time implementation ([#&#8203;153](marshallpierce/rust-base64#153), presumably `ConstantTimePortable`?) for security-sensitive applications that need side-channel resistance, and CPU-specific SIMD implementations for  more speed.
    -   Standard base64 per the RFC is available via `DEFAULT_ENGINE`. To use different alphabets or other settings (padding, etc), create your own engine instance.
-   `CharacterSet` is now `Alphabet` (per the RFC), and allows creating custom alphabets. The corresponding tables that were previously code-generated are now built dynamically.
-   Since there are already multiple breaking changes, various functions are renamed to be more consistent and discoverable.
-   MSRV is now 1.47.0 to allow various things to use `const fn`.
-   `DecoderReader` now owns its inner reader, and can expose it via `into_inner()`. For symmetry, `EncoderWriter` can do the same with its writer.
-   `encoded_len` is now public so you can size encode buffers precisely.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone01@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1683
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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

Successfully merging this pull request may close these issues.

None yet

2 participants