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

Jump to bottom

encoding/pem: should allow whitespace in the base64 body #54054

Open
jimsnab opened this issue Jul 25, 2022 · 4 comments
Open

encoding/pem: should allow whitespace in the base64 body #54054

jimsnab opened this issue Jul 25, 2022 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@jimsnab
Copy link

jimsnab commented Jul 25, 2022 •
edited

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

$ go version
go version go1.18.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import (
	"encoding/pem"
	"fmt"
)

func main() {

	p := `-----BEGIN CERTIFICATE-----
		MIIGATCCA+mgAwIBAgIUCn3
		 . . . valid pem data . . .
		ib7Jp7Q=
		-----END CERTIFICATE-----`

	blk, rest := pem.Decode([]byte(p))

	fmt.Printf("blk: %p\n", blk)
	fmt.Printf("rest len: %d\n", len(rest))
}

Paste valid PEM data in the example above. The whitespace indentation must be included. Typical output:

blk: 0x0
rest len: 2210

What did you expect to see?

Take the whitespace out of the PEM and see something like this:

blk: 0xc00010e180
rest len: 0

Per RFC 7468 says whitespace SHOULD be ignored. It's not a bug for this module to reject whitespace. But the RFC is absolutely clear that PEMs with whitespace can exist in the world, and is tolerated by some parsers, and I would expect this general purpose module to tolerate the whitespace as well.

Further, RFC 2045 is more explicit and says non-base64 characters MUST be ignored. Since the inner body of the PEM is base64, I would expect whitespace to be ignored by the golang parser.

What did you see instead?

Leading whitespace within the lines of the PEM octets causes the parser to fail.
@seankhliao seankhliao changed the title pem: should allow whitespace in the base64 body encoding/pem: should allow whitespace in the base64 body Jul 25, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 25, 2022
@seankhliao
Copy link
Member

cc @rsc

@ianlancetaylor
Copy link
Contributor

CC @FiloSottile @agl

@dsnet
Copy link
Member

dsnet commented Jul 26, 2022 •
edited

This is related to #53845, which proposes the opposite, where it locks down the set of allowed characters. We could modify that proposal to instead be something more versatile like:

// WithIgnored specifies a set of non-alphabet characters that are ignored
// when parsing the input. An empty string causes the encoder to reject
// all characters that are not part of the encoding alphabet.
// A newly created Encoder ignores '\r' and '\n' by default.
func (enc Encoding) WithIgnored(chars string) *Encoding

Thus, for my original purposes for #53845, you could do enc.WithIgnored("") to be in a strict mode that rejects any non-alphabet characters. On the other hand, for the use-cases of PEM, the package could do enc.WithIgnored("\t\v\f \r\n") based on the list specified in RFC 7468, section 2.

Even though RFC 2045 says:

Any characters outside of the base64 alphabet are to be ignored in base64-encoded data.

IMO, that's a dangerous semantic. If someone were accidentally using the base64 URL alphabet (RFC 4648, section 5.), which uses '-' and '_' instead of '+' and '/', then that would not be detected as an error, but would be silently parsed as valid data. Ignoring whitespace, on the other hand, has a more obviously correct semantic.

@dsnet dsnet mentioned this issue Jul 26, 2022
Open
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@gopherbot
Copy link

Change https://go.dev/cl/532295 mentions this issue: encoding: support WithIgnored in base32 and base64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@ianlancetaylor @dsnet @gopherbot @seankhliao @jimsnab