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

encoding/base64: integer overflow in (*Encoding).EncodedLen #20235

Open
ekiru opened this issue May 4, 2017 · 4 comments
Open

encoding/base64: integer overflow in (*Encoding).EncodedLen #20235

ekiru opened this issue May 4, 2017 · 4 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ekiru
Copy link

ekiru commented May 4, 2017

What version of Go are you using (go version)?

1.8.1

What operating system and processor architecture are you using (go env)?

linux/386

What did you do?

I called base64.RawStdEncoding.EncodedLen() with 1<<30 (2 to the 30th power) on a 32-bit platform, as in line 9 of https://play.golang.org/p/mV9110MxJ- , and base64.StdEncoding.EncodedLen() with 1<<31 / 4 * 3 (equal to 1,610,612,736) as in line 10 of the same example.

Line 10 also calls the method with the same argument on base64.StdEncoding for comparison.

What did you expect to see?

I expected EncodedLen() on RawStdEncoding to return a value close to that returned by StdEncoding for 1<<30I think 1,431,655,766 is the correct value, but I may have miscalculated) and EncodedLen() on StdEncoding to not return a negative encoded length for 1<<31 / 4 * 3 (perhaps to panic? I'm not certain what the appropriate behavior here is).

What did you see instead?

Instead the first example returned 0 and the second -2147483648. Neither of these values are sufficient to hold a base64 encoding of a string of the supplied lengths.

@bradfitz bradfitz added this to the Go1.9Maybe milestone May 4, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label May 4, 2017
@aead
Copy link
Contributor

aead commented May 13, 2017

This is (obviously) caused by an int overflow. Following EncodeLen function would make the behavior more consistent:

if enc.padChar == NoPadding {
      return int((int64(n)*8 + 5) / 6) // minimum # chars at 6 bits per char
}
return int((int64(n) + 2) / 3 * 4) // minimum # 4-char quanta, 3 bytes each

The question is: What does a negative length value mean / Should EncodeLen return a negative value?

@bradfitz
Copy link
Contributor

This has always been like this, right? Moving to Go 1.10 unless this is a regression.

At some point things are going to overflow. It's not obvious what we should do. Panic?

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe May 24, 2017
@aead
Copy link
Contributor

aead commented May 24, 2017

Yes, more or less since Go1 - so I think this should probably be moved to Go1.10 ... As you said, it's not obvious how we should handle overflowing properly. If EncodedLen() expected a slice the solution above would solve the overflow problem but a user can pass any values so doc / panic / doc+panic? I'm not sure - furthermore encoding/base32 does basically the same...

@iwdgo
Copy link
Contributor

iwdgo commented May 14, 2018

Both base32 and base64 have the same behavior with too large values for EncodingLen.
This behavior is in line with RFC 4648 which states

  1. Security Considerations
    When base encoding and decoding is implemented, care should be taken
    not to introduce vulnerabilities to buffer overflow attacks, or other
    attacks on the implementation. A decoder should not break on invalid
    input including, e.g., embedded NUL characters (ASCII 0).

The EncodedLen function in discussion is used by the encoder itself directly. It means that panic would occur in the encoder too and would not be in line with the RFC. Returning an error would have a similar effect.

The current behavior allows to test if the encoder will overflow by using len(src) as documentation states.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants