Bug: Proxy Protocol CRC32C verification failure

[tomciopp]

Thank you again for all the work done on shipping proxy protocol. I know you had asked for people to test this out and report any issues. I did that on my local machine (everything worked as planned) but I neglected to do so in my staging environment. When I did so, I received the following error message:

"Failed CRC32C verification in PROXY protocol binary header. (PP 2.2)"

If we follow the error trail back to the code we find that the error is being raised here https://github.com/ninenines/ranch/blob/master/src/ranch_proxy_header.erl#L501

I familiarized myself with this section of the docs (§ 2.2.3) https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

And http://erlang.org/doc/man/erlang.html#crc32-2

Based on my quick reading I think that the issue is that CRC32C remains a binary representation of a 32 bit integer instead of an integer, so it will never match properly. If that is indeed correct, I think the code should be changed to something like:

case erlang:crc32(1302506282, [Before, <<0:32>>, After]) of
  binary_to_integer(CRC32C) ->
      parse_tlv(Rest, Len, Info, Header);
  _ ->
    {error, 'Failed CRC32C verification in PROXY protocol binary header. (PP 2.2)'}
end;

I'm trying to get some more information from Amazon so that I can write a test to cover this case, but that may take a bit of time to process.

[essen]

No CRC32C is converted to an integer properly, that's what the notation is showing.

If you could log a complete PROXY header I could take a look.

[essen]

Note that there's a test for this at the end of the file, however I have not tested the checksum against another implementation because I did not know of any. So it's possible I did it wrong.

[tomciopp]

Ok so I spent a few hours digging into this issue and I believe the function call to erlang:crc32 is incorrect. This looks like an easy mistake to make since the naming is so similar. There are slight differences between crc32 and crc32c and it doesn't look like there is a built in solution for crc32c in erlang.

https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt (§ 2.2.3) https://tools.ietf.org/html/rfc4960#section-6.8 https://tools.ietf.org/html/rfc4960#appendix-B https://en.wikipedia.org/wiki/Cyclic_redundancy_check

I was able to test this using the test code provided by amazon here: https://github.com/aws/elastic-load-balancing-tools/blob/8dd8a909ddabf4107bfabb2cc3e0144da6f1ec86/proprot/tst/com/amazonaws/proprot/Compatibility_AwsNetworkLoadBalancerTest.java

Here is a copy of the proxy protocol header provided in that test file suitable for testing in erlang.

Packet = <<13, 10, 13, 10, 0, 13, 10, 81, 85, 73, 84, 10, 33, 17, 0, 84, 172, 31, 7, 113, 172, 31, 10, 31, 200, 242, 0, 80, 3, 0, 4, 232, 214, 137, 45, 234, 0, 23, 1, 118, 112, 99, 101, 45, 48, 56, 100, 50, 98, 102, 49, 53, 102, 97, 99, 53, 48, 48, 49, 99, 57, 4, 0, 36, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>.

If you call ranch_proxy_header:parse(Packet) you should see the same error message as I printed above.

"Failed CRC32C verification in PROXY protocol binary header. (PP 2.2)"

I spent some time looking at the code in gen_sctp but I couldn't find anything related to crc32c. I did some other searching and came across a C implementation by Mark Adler (of Adler32 and zlib fame) which may make sense for us to use as a NIF.

https://github.com/madler/brotli/blob/master/crc32c.c

Perhaps however we should petition the erlang maintainers on adding this as a BIF to the language as well. Let me know how you want to proceed.

[essen]

Damn. I should have looked up CRC32c.

Obviously an implementation in OTP would be great to have, but for the time being I'd settle for a small inefficient Erlang implementation. We're not checking too many bytes and we got a binary.

[tomciopp]

@essen I did some more research and I don't think this will be too bad. Give me a few hours and I think I can cook something up.

[essen]

Yes (https://github.com/sahlberg/libiscsi/blob/master/lib/crc32c.c). The table can be a tuple as well.

[tomciopp]

Ok so I wrote a working copy of this in elixir and I've attempted to port it over to erlang. Let me know if there are any syntax errors since I don't write in erlang particularly often.

Based on running the example header from AWS, we may need to update the data passed into the CRC32C check as I think it truncates the beginning of the header. (The check expects the "\r\n\r\n\0\r\nQUIT\n" info.)

https://gist.github.com/tomciopp/2d174f3960b6386e86167268b1a9875d

[essen]

The beginning of the header is always the same, that's why it truncates it. Instead of starting with 16#FFFFFFFF it'll start with whatever the CRC32C of the beginning of the header is, basically. The code has syntax errors of course! But no worries I'll figure it out tomorrow.

[tomciopp]

I don't know if I communicated my previous point properly, I think we are on the same page but want to be 100% sure. Given the AWS packet mentioned in the above to pass the CRC32C check the data should be passed in as such. (imagine there is a erlang:crc32c function)

$:  erlang:crc32c(<<13, 10, 13, 10, 0, 13, 10, 81, 85, 73, 84, 10, 33, 17, 0, 84, 172,31,7,113,172,31,10,31,200,242,0,80,3,0,4, 0, 0, 0, 0, 234, 0, 23, 1, 118, 112, 99, 101, 45, 48, 56, 100, 50, 98, 102, 49, 53, 102, 97, 99, 53, 48, 48, 49, 99, 57, 4, 0, 36, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>)
=> 3906373933

$: <<232, 214, 137, 45>> == 0xE8D6892D == 3906373933

[essen]

I'm talking about this: https://github.com/ninenines/ranch/blob/master/src/ranch_proxy_header.erl#L500

Same needs to be done for CRC32C, just it'll produce a different start value.

[essen]

What I'll do is create a new module ranch_crc32c but I will not document it immediately because I don't want to increase the minor version number. I think it could be documented later on though.

[essen]

I've pushed a commit fixing this. Please try it as soon as possible so I can issue a Ranch and Cowboy releases.

[tomciopp]

👍 I will run the changes on our staging box later today and make sure everything is working. I will report back after I'm done.

[tomciopp]

Everything looks good on my end!

[essen]

I've released Ranch 1.7.1. I am now waiting for the Cowboy 2.6.1 build to finish and if successful (meaning no new errors compared to last release) I will tag: https://buildkite.com/ninenines/cowboy/builds/322

Closing, thanks!