Error messages with newlines cause PROTOCOL_ERROR

[enisoc]

I just updated my project from gRPC 0.12 to 0.13, and did a go get -u google.golang.org/grpc and regenerated my protos. My tests started failing with errors like this:

code = 13 desc = "stream error: stream ID 1; PROTOCOL_ERROR"

It turns out it's because some of the error messages I return from streaming RPCs contain \n. If I remove the \n characters, the problem goes away.

I haven't made a reduced test case, but you can try taking any streaming RPC and adding return fmt.Errorf("error with\nnewline") on the server implementation.

[iamqizhao]

@bradfitz Can you take a look? This seems introduced by the http2.MetaHeadersFrame change. I tried to append a "\n" at the end of the error desc at https://github.com/grpc/grpc-go/blob/master/test/end2end_test.go#L97 and ran TestFailedEmptyUnary test case. It hangs there. Before the change of http2.MetaHeadersFrame, it seems work fine.

[bradfitz]

The http2.MetaHeadersFrame change exposed an existing problem but it is not the problem itself.

The http2 spec is very clear:

http://httpwg.org/specs/rfc7540.html#rfc.section.10.3

Similarly, HTTP/2 allows header field values that are not valid. While most of the values that can be encoded will not alter header field parsing, carriage return (CR, ASCII 0xd), line feed (LF, ASCII 0xa), and the zero character (NUL, ASCII 0x0) might be exploited by an attacker if they are translated verbatim. Any request or response that contains a character not permitted in a header field value MUST be treated as malformed (Section 8.1.2.6). Valid characters are defined by the field-content ABNF rule in Section 3.2 of [RFC7230].

grpc & grpc-go should not be putting user's raw error messages without escaping into header fields.

The grpc wire protocol says:

http://www.grpc.io/docs/guides/wire.html

Note that HTTP2 does not allow arbitrary octet sequences for header values so binary header values must be encoded using Base64 as per https://tools.ietf.org/html/rfc4648#section-4. Implementations MUST accept padded and un-padded values and should emit un-padded values. Applications define binary headers by having their names end with “-bin”. Runtime libraries use this suffix to detect binary headers and properly apply base64 encoding & decoding as headers are sent and received.

I think grpc-go might not handle the *-bin headers in enough cases? Maybe it's only implemented for metadata headers?

$ git grep -- -bin
metadata/metadata.go:   binHdrSuffix = "-bin"
metadata/metadata_test.go:              {"key-bin", "abc", "key-bin", "YWJj"},
metadata/metadata_test.go:              {"key-bin", binaryValue, "key-bin", "woA="},
metadata/metadata_test.go:              {"key-bin", "Zm9vAGJhcg==", "key-bin", "foo\x00bar", nil},
metadata/metadata_test.go:              {"key-bin", "woA=", "key-bin", binaryValue, nil},
metadata/metadata_test.go:              {[]string{"k1", "v1", "k2-bin", binaryValue}, New(map[string]string{
metadata/metadata_test.go:                      "k2-bin": binaryValue,

I'm happy to implement this if you'd like, @iamqizhao, or you can if you want to. Let me know.

[iamqizhao]

Thanks, Brad. Yeah, I have planned to add the error/status encoding/processing in. But in my experiments (as I mentioned in my first reply), the client hangs forever instead of returning any error. Can you take a look?

[bufdev]

Re 606, saying this is expected behavior - is it? I think grpc should be able to handle errors with newlines, without the end user caring about the transport - can't it just be escaped? @iamqizhao

[bradfitz]

@peter-edge, I believe there's already an open bug for transparent base64 escaping. If you can't find it, please open one. (I'm on a poor connection)

[bufdev]

I could use a good grpc bug to get into, lemme see what I can do :) but tomorrow, I am on Europe time

[bradfitz]

And I'm currently on Australia time. If we're distributed across the globe we'll have good code review latency with somebody always awake. :-)

[bufdev]

+1 for GOOG :)

[bufdev]

@bradfitz just did #609 which fixes the immediate issue, and at least allows newlines in error messages again (I'm getting this error in other repositories now too, no fun)

[bufdev]

#610

[hsaliak]

@peter-edge @enisoc are you still seeing this issue?

[enisoc]

I never found out the status of the fix. I just stripped all the newlines from my error messages.

[dfawley]

This should be fixed by #610