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/base32: decoder silently ignores unpadded trailing characters, depending on the chunking of the underlying Reader #25296

Closed
AxbB36 opened this issue May 8, 2018 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@AxbB36
Copy link

AxbB36 commented May 8, 2018

The Reader returned by base32.NewReader sometimes fails to signal an error when its input is improperly padded (length not a multiple of 4 bytes). Whether an error is signaled depends on how the underlying Reader segments its output. If its final read is aligned to a 4-byte boundary and contains only the final unpadded block, then the package does the (IMO) right thing and returns io.UnexpectedEOF. But otherwise, then the package silently ignores the extraneous characters and returns io.EOF (i.e., successful completion of decoding).

The example program demonstrates this with the 18-byte input "NBSWY3DPO5XXE3DEZZ". When the final read is of "ZZ", it returns io.UnexpectedEOF. But when the final read is "EZZ", "DEZZ", or anything else, it returns io.EOF. I would expect it to return io.UnexpectedEOF in all cases. (A case could be made for returning base32.CorruptInputError, like base32.DecodeString does, but at any rate the error should be something other than io.EOF.)

This bug was originally reported on golang-nuts, in 2014 for go1.3.2. There was some discussion but it never got fixed. The thread mentions that base64 may be similarly affected, but I didn't check.

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

go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes, with go1.10.2 on play.golang.org.

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

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

https://play.golang.org/p/Bya6YSpMJrB

package main

import (
	"encoding/base32"
	"fmt"
	"io"
	"io/ioutil"
)

func test(chunks []string) {
	fmt.Printf("\n")
	fmt.Printf("%q\n", chunks)

	pr, pw := io.Pipe()

	// Write the encoded chunks into the pipe.
	go func() {
		for _, chunk := range chunks {
			pw.Write([]byte(chunk))
		}
		pw.Close()
	}()

	// Decode base32 from the read end of the pipe.
	decoder := base32.NewDecoder(base32.StdEncoding, pr)
	data, err := ioutil.ReadAll(decoder)
	if err == nil {
		// Restore the EOF that ReadAll elides.
		err = io.EOF
	}
	fmt.Printf("%q %v\n", data, err)
}

func main() {
	// base32.CorruptInputError
	s, err := base32.StdEncoding.DecodeString("NBSWY3DPO5XXE3DEZZ")
	fmt.Printf("DecodeString(%q)\n", "NBSWY3DPO5XXE3DEZZ")
	fmt.Printf("%q %v\n", s, err)

	// io.UnexpectedEOF
	test([]string{"NBSW", "Y3DP", "O5XX", "E3DE", "ZZ"})
	test([]string{"NBSWY3DPO5XXE3DE", "ZZ"})

	// io.EOF
	test([]string{"NBSWY3DPO5XXE3DEZZ"})
	test([]string{"NBS", "WY3", "DPO", "5XX", "E3D", "EZZ"})
	test([]string{"NBSWY3DPO5XXE3", "DEZZ"})
}

What did you expect to see?

DecodeString("NBSWY3DPO5XXE3DEZZ")
"helloworld" illegal base32 data at input byte 16

["NBSW" "Y3DP" "O5XX" "E3DE" "ZZ"]
"helloworld" unexpected EOF

["NBSWY3DPO5XXE3DE" "ZZ"]
"helloworld" unexpected EOF

["NBSWY3DPO5XXE3DEZZ"]
"helloworld" unexpected EOF

["NBS" "WY3" "DPO" "5XX" "E3D" "EZZ"]
"helloworld" unexpected EOF

["NBSWY3DPO5XXE3" "DEZZ"]
"helloworld" unexpected EOF

What did you see instead?

DecodeString("NBSWY3DPO5XXE3DEZZ")
"helloworld" illegal base32 data at input byte 16

["NBSW" "Y3DP" "O5XX" "E3DE" "ZZ"]
"helloworld" unexpected EOF

["NBSWY3DPO5XXE3DE" "ZZ"]
"helloworld" unexpected EOF

["NBSWY3DPO5XXE3DEZZ"]
"helloworld" EOF

["NBS" "WY3" "DPO" "5XX" "E3D" "EZZ"]
"helloworld" EOF

["NBSWY3DPO5XXE3" "DEZZ"]
"helloworld" EOF
@josharian
Copy link
Contributor

cc @zegl

@josharian josharian changed the title base32 decoder silently ignores unpadded trailing characters, depending on the chunking of the underlying Reader encoding/base32: decoder silently ignores unpadded trailing characters, depending on the chunking of the underlying Reader May 8, 2018
@josharian josharian added this to the Go1.11 milestone May 8, 2018
zegl added a commit to zegl/go that referenced this issue May 9, 2018
This changes decoder.Read to always return io.ErrUnexpectedEOF if the input
contains surplus padding or unexpected content. Previously the error could
be io.EOF or io.ErrUnexpectedEOF depending on how the input was chunked.

Fixes golang#25296
@gopherbot
Copy link

Change https://golang.org/cl/112516 mentions this issue: encoding/base32: handle surplus padding consistently

@zegl
Copy link
Contributor

zegl commented May 9, 2018

Changing the behavior of decoder.Read is a bit risky, as some inputs will now cause ioutil.ReadAll to return a non-nil error where it previously returned nil. I definitely think that the CL should be merged anyway, as the current behavior is very surprising.

Thanks for two very nice bug reports @AxbB36! 👏

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label May 9, 2018
@golang golang locked and limited conversation to collaborators May 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants