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

Incorrect base64 implementation? #1277

Closed
david-perez opened this issue Mar 24, 2022 · 0 comments
Closed

Incorrect base64 implementation? #1277

david-perez opened this issue Mar 24, 2022 · 0 comments
Assignees
Milestone

Comments

@david-perez
Copy link
Contributor

Consider this patch:

diff --git a/rust-runtime/aws-smithy-types/src/base64.rs b/rust-runtime/aws-smithy-types/src/base64.rs
index 846c7eb1..5c49b708 100644
--- a/rust-runtime/aws-smithy-types/src/base64.rs
+++ b/rust-runtime/aws-smithy-types/src/base64.rs
@@ -196,6 +196,19 @@ mod test {
         assert_eq!(encode("anything you wan"), "YW55dGhpbmcgeW91IHdhbg==");
     }

+    #[test]
+    fn test_base64_misc() {
+        let us_1 = decode("xyz");
+        let them_1 = base64::decode("xyz");
+        dbg!(&us_1);
+        dbg!(&them_1);
+
+        let us_2 = decode("YmxvYg=");
+        let them_2 = base64::decode("YmxvYg=");
+        dbg!(&us_2);
+        dbg!(&them_2);
+    }
+
     #[test]
     fn test_invalid_padding() {
         // no internal padding

cargo test test_base64_misc -- --nocapture yields:

running 1 test
[aws-smithy-types/src/base64.rs:203] &us_1 = Ok(
    [
        199,
        44,
    ],
)
[aws-smithy-types/src/base64.rs:204] &them_1 = Err(
    InvalidLastSymbol(
        2,
        122,
    ),
)
[aws-smithy-types/src/base64.rs:208] &us_2 = Ok(
    [
        98,
        108,
        111,
        98,
    ],
)
[aws-smithy-types/src/base64.rs:209] &them_2 = Ok(
    [
        98,
        108,
        111,
        98,
    ],
)
test base64::test::test_base64_misc ... ok

So the first case seems to be invalid but our implementation is accepting it. The second case is technically not valid base64 (note code point length is not divisible by 4), since it's missing an extra padding = character, but I'm guessing both implementations can unambiguously decode it. However, other implementations are stricter:

>> atob("YmxvYg=");
Uncaught DOMException: String contains an invalid character

These two cases are deemed as invalid base64 according to Smithy protocol tests.

david-perez added a commit that referenced this issue Apr 1, 2022
This removes our implementation of base64 encoding/decoding from
`aws-smithy-types`, switching to the `base64` Rust crate.

Our implementation was written at a time when we wanted to keep
dependencies at an absolute minimum. However, a performant base64
implementation is now needed for the server.

Our implementation was also accepting invalid base64 (see #1277).

Fixes #1277.
@david-perez david-perez mentioned this issue Apr 1, 2022
2 tasks
@spinski spinski added this to the Server GA milestone Nov 24, 2022
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 a pull request may close this issue.

4 participants