diff options
author | Tor Brede Vekterli <vekterli@oath.com> | 2018-09-25 14:17:23 +0000 |
---|---|---|
committer | Tor Brede Vekterli <vekterli@oath.com> | 2018-09-25 14:17:23 +0000 |
commit | 6306d72742feba81038761b9ce8aa6bd76fd088c (patch) | |
tree | 56a740f7b26509f8b6b036bc48eba11d256a25ad /vespalib | |
parent | bcfae9e52bb65ca165fb659971a0c6e4c0a0a3cc (diff) |
Address code review feedback
Diffstat (limited to 'vespalib')
3 files changed, 26 insertions, 12 deletions
diff --git a/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp b/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp index e4cf06f6631..2d203047835 100644 --- a/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp +++ b/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp @@ -3,7 +3,7 @@ #include <vespa/vespalib/net/tls/protocol_snooping.h> using namespace vespalib; -using namespace vespalib::net::tls; +using namespace vespalib::net::tls::snooping; TEST("min_header_bytes_to_observe() is 8") { EXPECT_EQUAL(8u, min_header_bytes_to_observe()); @@ -28,9 +28,14 @@ TEST("Mismatching handshake header byte 1 returns HandshakeMismatch") { EXPECT_EQUAL(TlsSnoopingResult::HandshakeMismatch, do_snoop(buf)); } -TEST("Mismatching handshake header byte 2 returns HandshakeMismatch") { +TEST("Mismatching major version byte returns ProtocolVersionMismatch") { const unsigned char buf[] = { 22, 2, 1, 10, 255, 1, 0, 10 }; - EXPECT_EQUAL(TlsSnoopingResult::HandshakeMismatch, do_snoop(buf)); + EXPECT_EQUAL(TlsSnoopingResult::ProtocolVersionMismatch, do_snoop(buf)); +} + +TEST("Mismatching minor version byte returns ProtocolVersionMismatch") { + const unsigned char buf[] = { 22, 3, 0, 10, 255, 1, 0, 10 }; + EXPECT_EQUAL(TlsSnoopingResult::ProtocolVersionMismatch, do_snoop(buf)); } TEST("Oversized record returns RecordSizeRfcViolation") { @@ -42,6 +47,11 @@ TEST("Oversized record returns RecordSizeRfcViolation") { EXPECT_EQUAL(TlsSnoopingResult::RecordSizeRfcViolation, do_snoop(buf2)); } +TEST("Undersized record returns RecordSizeRfcViolation") { + const unsigned char buf1[] = { 22, 3, 1, 0, 3, 1, 0, 0 }; + EXPECT_EQUAL(TlsSnoopingResult::RecordSizeRfcViolation, do_snoop(buf1)); +} + TEST("Non-ClientHello handshake record returns RecordNotClientHello") { const unsigned char buf[] = { 22, 3, 1, 10, 255, 2, 0, 10 }; // ^ 1 == ClientHello @@ -60,4 +70,9 @@ TEST("Expected ClientHello record size mismatch returns ExpectedRecordSizeMismat EXPECT_EQUAL(TlsSnoopingResult::ExpectedRecordSizeMismatch, do_snoop(buf)); } +TEST("Valid ClientHello record size with LSB < 4 returns ProbablyTls") { + const unsigned char buf[] = { 22, 3, 1, 10, 3, 1, 0, 9 }; + EXPECT_EQUAL(TlsSnoopingResult::ProbablyTls, do_snoop(buf)); +} + TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp index 04a0a29be64..6ccfc7f27ac 100644 --- a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp +++ b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp @@ -4,7 +4,7 @@ #include <cstdlib> #include <stdint.h> -namespace vespalib::net::tls { +namespace vespalib::net::tls::snooping { namespace { @@ -12,17 +12,17 @@ namespace { // From RFC 5246: // 0x16 - Handshake content type byte of TLSCiphertext record -// 0x03 - First byte of 2-byte ProtocolVersion, always 3 on TLSv1.2 and v1.3 inline bool is_tls_handshake_packet(const char* buf) { - return ((buf[0] == 0x16) && (buf[1] == 0x03)); + return (buf[0] == 0x16); } +// First byte of 2-byte ProtocolVersion, always 3 on TLSv1.2 and v1.3 // Next is the TLS minor version, either 1 or 3 depending on version (though the // RFCs say it _should_ be 1 for backwards compatibility reasons). // Yes, the TLS spec says that you should technically ignore the protocol version // field here, but we want all the signals we can get. inline bool is_expected_tls_protocol_version(const char* buf) { - return ((buf[2] == 0x01) || (buf[2] == 0x03)); + return ((buf[1] == 0x03) && ((buf[2] == 0x01) || (buf[2] == 0x03))); } // Length is big endian u16 in bytes 3, 4 @@ -70,7 +70,7 @@ TlsSnoopingResult snoop_client_hello_header(const char* buf) noexcept { // where we control frame sizes and where such fragmentation should not take place. // We also do not support TLSv1.3 0-RTT which may trigger early data. uint16_t length = tls_record_length(buf); - if (length > (16384 + 2048)) { + if ((length < 4) || (length > (16384 + 2048))) { return TlsSnoopingResult::RecordSizeRfcViolation; } if (!is_client_hello_handshake_record(buf)) { @@ -95,8 +95,8 @@ const char* to_string(TlsSnoopingResult result) noexcept { case TlsSnoopingResult::RecordNotClientHello: return "RecordNotClientHello"; case TlsSnoopingResult::ClientHelloRecordTooBig: return "ClientHelloRecordTooBig"; case TlsSnoopingResult::ExpectedRecordSizeMismatch: return "ExpectedRecordSizeMismatch"; - default: return "Unknown TlsSnoopingResult"; } + abort(); } std::ostream& operator<<(std::ostream& os, TlsSnoopingResult result) { @@ -120,9 +120,8 @@ const char* describe_result(TlsSnoopingResult result) noexcept { return "ClientHello record is too big (fragmented?)"; case TlsSnoopingResult::ExpectedRecordSizeMismatch: return "ClientHello vs Handshake header record size mismatch"; - default: - return "Unknown TlsSnoopingResult"; } + abort(); } } diff --git a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h index 59f6ecf6e9d..f53e136e597 100644 --- a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h +++ b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h @@ -4,7 +4,7 @@ #include <iosfwd> #include <stddef.h> -namespace vespalib::net::tls { +namespace vespalib::net::tls::snooping { constexpr inline size_t min_header_bytes_to_observe() { return 8; } |