code review 75340044: base64: fix bug that decoder fails to detect corruption

base64: fix bug that decoder fails to detect corruption

Encoding.Decode() failed to detect trailing garbages if input contains "==" followed by garbage smaller than 3 bytes (for example, it failed to detect "x" in "AA==x"). This patch fixes the bug and adds a few tests.

[ruiu, 2014-03-17 07:29:32]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-03-17 18:22:38]

R=nigeltao



On Mon, Mar 17, 2014 at 12:29 AM, <ruiu@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> encode/base64: fix bug that decoder fails to detect corruption.
>
> Encoding.Decode() failed to detect trailing garbages if input contains
> "=="
> followed by garbage smaller than 3 bytes (for example, it failed to
> detect
> "x" in "AA==x"). This patch fixes the bug and adds a few tests.
>
> Please review this at https://codereview.appspot.com/75340044/
>
> Affected files (+24, -9 lines):
>   M src/pkg/encoding/base64/base64.go
>   M src/pkg/encoding/base64/base64_test.go
>
>
> Index: src/pkg/encoding/base64/base64.go
> ===================================================================
> --- a/src/pkg/encoding/base64/base64.go
> +++ b/src/pkg/encoding/base64/base64.go
> @@ -226,21 +226,35 @@
>                 var dbuf [4]byte
>                 dlen := 4
>
> -               for j := 0; j < 4; {
> +               for j := 0; j < 4; j++ {
>                         if len(src) == 0 {
>                                 return n, false, CorruptInputError(olen -
> len(src) - j)
>                         }
>                         in := src[0]
>                         src = src[1:]
> -                       if in == '=' && j >= 2 && len(src) < 4 {
> +                       if in == '=' {
>                                 // We've reached the end and there's
> padding
> -                               if len(src)+j < 4-1 {
> -                                       // not enough padding
> -                                       return n, false,
> CorruptInputError(olen)
> -                               }
> -                               if len(src) > 0 && src[0] != '=' {
> +                               switch j {
> +                               case 0:
> +                                       fallthrough
> +                               case 1:
>                                         // incorrect padding
>                                         return n, false,
> CorruptInputError(olen - len(src) - 1)
> +                               case 2:
> +                                       // "==" is expected, the first "="
> is already consumed.
> +                                       if len(src) == 0 {
> +                                               // not enough padding
> +                                               return n, false,
> CorruptInputError(olen)
> +                                       }
> +                                       if src[0] != '=' {
> +                                               // incorrect padding
> +                                               return n, false,
> CorruptInputError(olen - len(src) - 1)
> +                                       }
> +                                       src = src[1:]
> +                               }
> +                               if len(src) > 0 {
> +                                       // trailing garbage
> +                                       return n, false,
> CorruptInputError(olen - len(src))
>                                 }
>                                 dlen, end = j, true
>                                 break
> @@ -249,7 +263,6 @@
>                         if dbuf[j] == 0xFF {
>                                 return n, false, CorruptInputError(olen -
> len(src) - 1)
>                         }
> -                       j++
>                 }
>
>                 // Pack 4x 6-bit source blocks into 3 byte destination
> Index: src/pkg/encoding/base64/base64_test.go
> ===================================================================
> --- a/src/pkg/encoding/base64/base64_test.go
> +++ b/src/pkg/encoding/base64/base64_test.go
> @@ -149,9 +149,11 @@
>         }{
>                 {"", -1},
>                 {"!!!!", 0},
> +               {"====", 0},
>                 {"x===", 1},
>                 {"AA=A", 2},
> -               {"AAA=AAAA", 3},
> +               {"AA==A", 4},
> +               {"AAA=AAAA", 4},
>                 {"AAAAA", 4},
>                 {"AAAAAA", 4},
>                 {"A=", 1},
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[nigeltao, 2014-03-18 05:50:10]

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
File src/pkg/encoding/base64/base64.go (right):

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
src/pkg/encoding/base64/base64.go:229: for j := 0; j < 4; j++ {
This can now be
for j := range dbuf {

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
src/pkg/encoding/base64/base64.go:238: case 0:
case 0, 1:

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
File src/pkg/encoding/base64/base64_test.go (right):

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
src/pkg/encoding/base64/base64_test.go:163: {"AAA=", -1},
I think that it's worth adding these test cases:
"AA=A"
"A=AA"
"=AAA"

[ruiu, 2014-03-18 17:48:32]

PTAL.

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
File src/pkg/encoding/base64/base64.go (right):

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
src/pkg/encoding/base64/base64.go:229: for j := 0; j < 4; j++ {
On 2014/03/18 05:50:10, nigeltao wrote:
> This can now be
> for j := range dbuf {

Done.

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
src/pkg/encoding/base64/base64.go:238: case 0:
On 2014/03/18 05:50:10, nigeltao wrote:
> case 0, 1:

Done.

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
File src/pkg/encoding/base64/base64_test.go (right):

https://codereview.appspot.com/75340044/diff/40001/src/pkg/encoding/base64/ba...
src/pkg/encoding/base64/base64_test.go:163: {"AAA=", -1},
On 2014/03/18 05:50:10, nigeltao wrote:
> I think that it's worth adding these test cases:
> "AA=A"
> "A=AA"
> "=AAA"

Done.

[nigeltao, 2014-03-20 05:00:09]

LGTM.

[nigeltao, 2014-03-20 05:00:45]

*** Submitted as https://code.google.com/p/go/source/detail?r=7aeb36fc114b ***

base64: fix bug that decoder fails to detect corruption

Encoding.Decode() failed to detect trailing garbages if input contains "=="
followed by garbage smaller than 3 bytes (for example, it failed to detect "x"
in "AA==x"). This patch fixes the bug and adds a few tests.

LGTM=nigeltao
R=golang-codereviews, bradfitz, nigeltao
CC=golang-codereviews
https://codereview.appspot.com/75340044

Committer: Nigel Tao <nigeltao@golang.org>