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.

Sign up for GitHub

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

Add RFC 8422 Point Format Extension tests #796

Open

cipherboy wants to merge 1 commit into tlsfuzzer:master from cipherboy:add-rfc-8422-tests

Contributor

cipherboy commented Jul 28, 2022 •

edited

Description

This adds tests for the Point Format Extension into tlsfuzzer. In
addition to a basic sanity check, we include:

A hacked extension with no point formats specified; (fail)
A handshake with no extension present; (pass)
A handshake with all formats present; (pass) and
A handshake with only ANSI format present (fail).

Notably, the latter only occurs on RFC 8422-compliant TLS
implementations.

NSS correctly conforms to the behavior, yielding PASS: 4. However,
OpenSSL/GnuTLS fails the only ANSI test, returning a ServerHello with all
three point formats (including the unsupported uncompressed format);
this could be desired behavior since OpenSSL doesn't know the peer
is RFC 8422 compliant. Go's default TLS configuration fails only the
no extension check, indicating a bug in the Go TLS implementation.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Motivation and Context

We had reports of Go's TLS implementation failing to handshake with certain old TLSv1.2 clients; after a bit of debugging, we landed on scenario 2 being the culprit and will begin filing issue/patches upstream. However, it was necessary to build out separate testing infra for reproducing this issue so I opted for tlsfuzz.

Checklist

This change is


          Add RFC 8422 Point Format Extension tests

b026b5b

This adds tests for the Point Format Extension into tlsfuzzer. In
addition to a basic sanity check, we include:

 1. A hacked extension with no point formats specified; (fail)
 2. A handshake with no extension present; (pass)
 3. A handshake with all formats present; (pass) and
 4. A handshake with only ANSI format present (fail).

Notably, the latter only occurs on RFC 8422-compliant TLS
implementations.

NSS correctly conforms to the behavior, yielding PASS: 4. However,
OpenSSL and GnuTLS fails the only ANSI test, returning a ServerHello
with all three point formats (including the unsupported uncompressed
format); this could be desired behavior since OpenSSL/GnuTLS doesn't
know if the peer is RFC 8422 compliant. Go's default TLS configuration
fails only the no extension check, indicating a bug in the Go TLS
implementation.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

This was referenced Jul 28, 2022

Add initial Go TLS server for testing #797

Closed

crypto/tls: Failure to handshake when ClientHello lacks Point Formats Extension golang/go#54116

Closed

tomato42 requested changes

View reviewed changes

Member

tomato42 left a comment

Also, please note that all scripts must be added to the tests/tlslite-ng.json and tests/tlslite-ng-random-subset.json scripts (CI failures will soon show that too).

scripts/test-tls12-point-format-extension.py



		def main():
		"""check if app data records with zero payload are accepted by server"""

Member

tomato42 Jul 28, 2022

I don't think this applies to this script...
also, in general the scripts/test-conversation.py is the better template to use for creating new test scripts

scripts/test-tls12-point-format-extension.py

+                      SignatureAlgorithmsCertExtension().create(SIG_ALL)
+                  ext_missing = ext.copy()
+                  # Extension present, no point formats supported.

Member

tomato42 Jul 28, 2022

The below will create a malformed extension, the syntax specifies the contents as

ECPointFormat ec_point_format_list<1..2^8-1>

Which mean that an empty list would be encoded as b"\x00", not b"". The latter specifies a missing list.

Contributor Author

cipherboy Jul 28, 2022

Ah, d'oh. Yes, that's a good fix.

scripts/test-tls12-point-format-extension.py

+                  conversation = Connect(host, port)
+                  node = conversation
+                  node = node.add_child(ClientHelloGenerator(ciphers, extensions=ext_ansi))
+                  node = node.add_child(ExpectAlert())

Member

tomato42 Jul 28, 2022

any reason not to specify specific alert to expect?

scripts/test-tls12-point-format-extension.py

+                  node = node.add_child(ClientHelloGenerator(ciphers, extensions=ext_ansi))
+                  node = node.add_child(ExpectAlert())
+                  node.next_sibling = ExpectClose()
+                  node.add_child(Close())

Member

tomato42 Jul 28, 2022

This construction means: expect an Alert, or connection close; if an Alert is received, close the connection.
Shouldn't we rather wait for Alert followed by close? i.e. there should be two ExpectClose() nodes.

scripts/test-tls12-point-format-extension.py

+                  node = node.add_child(ExpectAlert())
+                  node.next_sibling = ExpectClose()
+                  node.add_child(Close())
+                  conversations["all_formats"] = conversation

Member

tomato42 Jul 28, 2022

all tests use plain language for names, with spaces instead of underscores...

tomato42 added the enhancement label

tomato42 added this to In progress in TLS extensions via automation

Member

tomato42 commented Jul 28, 2022

the changes are also reflected in documentation and code comments -- let me know if I need to doc these somewhere.

it's about new features in the tlsfuzzer proper, new test scripts don't require any extra documentation

let me know if you want me to add more scenarios; I don't think versions matter too much here, though this was discovered with a TLSv1.2 client (and the RFC in particular is less useful in a TLSv1.3 context) --- renegotiation I don't think is impactful, but let me know if you want more invalid format tests &c.

IIRC TLS1.3 is a bit more strict about it, but I'd really have to check; in general, I won't insist on a second script for TLS 1.3, TLS1.2-only test coverage is fine. I think there is one case missing: an empty list (vs the empty extension that's already covered). Renegotiation may be impactful in the "what if it's removed or added", implementations commonly trip over it.
Undefined codepoints may be interesting, Same for duplicated codepoints, large lists.
Testing with padded or truncated values is also applicable.

That being said, those are things you can do, if your goal is complete test coverage for this extension. If you're after just checking those few cases you already implemented, that's fine too.
Though I think we should still add the empty array case, as I think that was your intention from the beginning...

new test script added to tlslite-ng.json and tlslite-ng-random-subset.json --> need help with this one @tomato42 :-)

as I mentioned in the comment above, look into tests/ for those files, it should be pretty self explanatory what to do :)

What's less obvious, is that you can execute those json files with something like

PYTHONPATH=. python3 tests/scripts_retention.py tests/tlslite-ng-random-subset.json ../tlslite-1/scripts/tls.py 1852

(the 1852 is the expected size of server response when running on python 3.10, use 1850 on python 2.7, 1849 on python 3.8.6, etc. it's used for the test-record-size-limit.py script)

Member

tomato42 commented Jul 28, 2022

test-tls12-point-format-extension.py

One more thing: all tests that apply to tls1.2 and earlier don't include the version in the name, only tls 1.3 specific tests do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment