statem: fix the alert sent for too large messages

[tomato42]

fixes #8293

Checklist

• documentation is added or updated
• tests are added or updated (verified with Incorrect alert for ClientHello with data after extensions field #8293)

[davidben]

This does not seem to have anything to do with the specification text cited in #8293, which is talking about a specific syntax error. As noted in #8189 (comment), decode_error is incorrect in the general case for this DoS check, which may overlap with any number of inputs, valid or invalid against the formal grammar.

[tomato42]

This does not seem to have anything to do with the specification text cited in #8293,

if you're saying that the test case is incorrect, I'd appreciate a bit more precision in stating how it is not creating a ClientHello with data following extensions field

decode_error is incorrect in the general case for this DoS check

which section of RFC states that? the only reference to DoS in RFC 8446 I could find is in relation to cookie extension

[davidben]

The problem is your test case does not consider inputs where multiple things may be wrong. In that case, the error emitted is a function of processing order. Consider instead a Certificate message which exceeds max_cert_list, is completely syntactically valid, and has an unsolicited extension. The specification says the correct alert in that scenario is unsupported_extension, yet your PR is using decode_error.

It is true that the specification does not include text for implementation-defined limits on message sizes. This is perhaps an error in the specification. They are necessary for any production TLS implementation.

Fixating on that input for this code is quite unproductive, as this code is clearly not implementing the scenario you are describing. It is checking an additional local policy in the TLS implementation. You must realize this, as the actual PR to achieve your goals here would be to remove the check entirely. But this is, of course, a bad idea for DoS reasons, so you haven't done this.

I assume your goal here is actually to smooth over network-visible differences between TLS implementations. That is a fine goal, but hopeless for TLS as it is. There are far too many low-level error scenarios with different alerts for the choice of alert to be unambiguous. A more constructive path towards this goal would be to produce a I-D to fold together all these needlessly separate alerts. The following should be one alert:

• decode_error
• illegal_parameter
• unsupported_extension
• unexpected_message

and perhaps others. I would happily support such a document. Arguing the philosophical differences between decode_error and illegal_parameter is a waste of time. I regret my part in trying to pin down their meanings for TLS 1.3. That was clearly a mistake.

[tomato42]

The problem is your test case does not consider inputs where multiple things may be wrong.

so the test case is wrong because it is testing the exact scenario described in the RFC?!

Certificate message which exceeds max_cert_list

max_cert_list is undefined in the RFC, what do you mean by it?

It is true that the specification does not include text for implementation-defined limits on message sizes. This is perhaps an error in the specification. They are necessary for any production TLS implementation.

so you're saying that by changing that alert I'm changing behaviour that is not sanctioned by the RFC? How is that a problem?

Fixating on that input for this code is quite unproductive

as this code is clearly not implementing the scenario you are describing.

That is a fine goal, but hopeless for TLS as it is.

You must realize this,

please, can you stop attacking me and focus on the code?

I regret my part in trying to pin down their meanings for TLS 1.3. That was clearly a mistake.

I'm sorry to hear that, because as a software tester I find them invaluable. And I definitely don't mean in the sense of generating easy bugs.

[davidben]

max_cert_list is undefined in the RFC, what do you mean by it?

It is defined in OpenSSL's implementation. See here:
https://github.com/openssl/openssl/blob/master/ssl/statem/statem_srvr.c#L1131

OpenSSL has a configurable maximum certificate size. This limit is smaller than what the syntax of Certificate would otherwise prescribe and, as you say, is not sanctioned by the RFC. That limit is enforced by exactly the code this PR changes.

That means one can construct inputs which trip this that additionally trip pretty much any other Certificate error condition, the exact same sort of scenario that your test input is targeting. By this PR's interpretation (unsanctioned limits cannot override the alert to send), there is no single alert one could possibly return from that codepath which would satisfy the specification.

[kroeckx]

I feel like we're repeating the same over and over again. The code that was change did not have a problem parsing the protocol, for that code it seems wrong to return decode_error.

The code that was changed also does not detect that there was data after the client hello, it just detects that the size is larger than some limit that's not in the RFC. I can't actually find a good error to send in that case, illegal_parameter seems like the closest match.

Note that the test triggers behaviour that's not in the RFC, and so I think there is little point in changing the alert. You can probably still detect this behaviour regardless of what the actual alert is that is being send.

Anyway, I think you should limit the amount of data that you send after the client hello so that the maximum size is not triggered and that parsing the data you send fails instead.

[tomato42]

The code that was change did not have a problem parsing the protocol, for that code it seems wrong to return decode_error.

how is that relevant? you can write code that is able to parse and interpret all kinds of malformed messages, just because a particular implementation is able to do something with it, doesn't mean that the standard prescribed alert should not be sent

The code that was changed also does not detect that there was data after the client hello, it just detects that the size is larger than some limit that's not in the RFC.

there is no valid ClientHello of that size that can be created without adding data after extensions field, ergo checking the length of this kind of messages is enough to say that they are malformed

Note that the test triggers behaviour that's not in the RFC, and so I think there is little point in changing the alert.

thing is, it is breaking behaviour that is specified by the RFC, see section quoted in #8293

Anyway, I think you should limit the amount of data that you send after the client hello so that the maximum size is not triggered and that parsing the data you send fails instead.

please check again the tlsfuzzer output, I think you must have missed few things there while reading it the first time

[tomato42]

That means one can construct inputs which trip this that additionally trip pretty much any other Certificate error condition,

but I'm not arguing what is the correct handling of messages that have multiple errors in them, you are, and you are not providing any standard references to back need for any particular behaviour in such cases

the exact same sort of scenario that your test input is targeting.

I'm sure that if you study the test case again, you will notice that this is not what it is doing

By this PR's interpretation (unsanctioned limits cannot override the alert to send), there is no single alert one could possibly return from that codepath which would satisfy the specification.

well, duh? rejecting large Certificate messages is not allowed by the standard so of course there is no standard prescribed alert message for that condition. There is nothing to reconcile from specification perspective.

I do agree that having limit on Certificate message is something that is an acceptable DoS mitigation and acceptable departure from the RFC.
But I will argue that sending decode_error is more logical in such situation (or more likely to lead the person debugging the issue on right track) as it points to the problem with the message itself rather one particular value in it. I don't see any place in the RFC 8446 in which illegal_parameter is specified as the alert to send when a field is too large.

But this is just my opinion, not a fact. While the fact that current OpenSSL behaviour is inconsistent with the RFC remains.

[mspncp]

‚send‘ -> ‚sent‘ (commit message and pr title)

[tomato42]

@mspncp good catch, updated

[t8m]

With the current definition of the maximum acceptable length of client hello it
is true that the decode_error would be more correct for excessive
client hello messages. However this code is used also for other
messages and there the limits are implementation defined and not
directly coming from the spec. For example for client key exchange
message the limit is set to 2048 bytes but the spec allows for up to
65535 bytes of public DH value and thus returning invalid_parameter
here is more appropriate for that case. In other message cases this is
varying - sometimes decode_error and sometimes invalid_parameter would
be more appropriate.

So I do not think the patch as is is correct. It would have to special-case the messages for which the limit is derived from the spec and for these return decode_error and for the rest return invalid_parameter. I am not sure it is worth it to complicate the code.

[tomato42]

With the current definition of the maximum acceptable length of client hello it
is true that the decode_error would be more correct for excessive
client hello messages. However this code is used also for other
messages and there the limits are implementation defined and not
directly coming from the spec. For example for client key exchange
message the limit is set to 2048 bytes but the spec allows for up to
65535 bytes of public DH value and thus returning invalid_parameter
here is more appropriate for that case.

that looks more like a bug than like something what we want to leave in:

1. FFDHE 8192 is now supported by OpenSSL, 2048 byte limit means it cannot handle it together with 8192 bit RSA keys
2. post quantum key shares will likely be larger
3. which part of the RFC states that implementation that refuses to process a message because of protocol-unrelated limits should reject it with invalid_parameter?

In other message cases this is
varying - sometimes decode_error and sometimes invalid_parameter would
be more appropriate.

cases such like?

So I do not think the patch as is is correct. It would have to special-case the messages for which the limit is derived from the spec and for these return decode_error and for the rest return invalid_parameter. I am not sure it is worth it to complicate the code.

yes, this PR is not making the situation perfect, but I think it improves it. If you don't think so, let's go through the cases and see if this is the case or not.

[richsalz]

The reason for the limit is to prevent DoS attacks:

FFDHE 8192 is now supported by OpenSSL, 2048 byte limit means it cannot handle it together with 8192 bit RSA keys

Yes, exactly. Increase the lmit if your system needs to handle this. 8K RSA keys are ridiculous in production environments.

post quantum key shares will likely be larger

When OpenSSL supports quantum-safe algorithms, then something will have to be done. but it doesn't, so we don't now. ;0

[tomato42]

The reason for the limit is to prevent DoS attacks:

FFDHE 8192 is now supported by OpenSSL, 2048 byte limit means it cannot handle it together with 8192 bit RSA keys

Yes, exactly. Increase the lmit if your system needs to handle this. 8K RSA keys are ridiculous in production environments.

not any more "ridiculous" than 8k DH keys, also it's inconsistent with how such keys behave on server side, or with different key exchange methods

post quantum key shares will likely be larger

When OpenSSL supports quantum-safe algorithms, then something will have to be done. but it doesn't, so we don't now. ;0

except people are already doing tests to check how PQC integrates with existing protocols and are modifying openssl to do that; fighting such arbitrary limitations makes that process much more frustrating

anyway, point 3 stands

and RFC still states

   If negotiating a version of TLS prior to 1.3,
   a server MUST check that the message either contains no data after
   legacy_compression_methods or that it contains a valid extensions
   block with no data following.  If not, then it MUST abort the
   handshake with a "decode_error" alert.

and

   In the rest of
   this specification, when the phrases "terminate the connection" and
   "abort the handshake" are used without a specific alert it means that
   the implementation SHOULD send the alert indicated by the
   descriptions below.  The phrases "terminate the connection with an X
   alert" and "abort the handshake with an X alert" mean that the
   implementation MUST send alert X if it sends any alert.

[t8m]

So I was convinced by Hubert that this patch improves the situation in regards to testing of the RFC conformance of the response to incorrect ClientHello and it does not make the situation worse in other cases. I am thus approving this. Please @openssl/omc consider approving it.

[kaduk]

Hubert, that quote from the RFC is predicated on the server "negotiating a version of TLS prior to 1.3". If the server decides that a ClientHello looks bogus and will be rejected because it's too big, it has not even gotten to the point of negotiating a version yet, and so it's not clear that the following requirement applies.

[tomato42]

@kaduk
first: this is just a fragment of that paragraph, there is a second "if" just before the quoted sentence, with situation that applies to TLS 1.3, the "if not" applies when neither is the case:

   All versions of TLS allow an extensions field to optionally follow
   the compression_methods field.  TLS 1.3 ClientHello messages always
   contain extensions (minimally "supported_versions", otherwise, they
   will be interpreted as TLS 1.2 ClientHello messages).  However,
   TLS 1.3 servers might receive ClientHello messages without an
   extensions field from prior versions of TLS.  The presence of
   extensions can be detected by determining whether there are bytes
   following the compression_methods field at the end of the
   ClientHello.  Note that this method of detecting optional data
   differs from the normal TLS method of having a variable-length field,
   but it is used for compatibility with TLS before extensions were
   defined.  TLS 1.3 servers will need to perform this check first and
   only attempt to negotiate TLS 1.3 if the "supported_versions"
   extension is present.  If negotiating a version of TLS prior to 1.3,
   a server MUST check that the message either contains no data after
   legacy_compression_methods or that it contains a valid extensions
   block with no data following.  If not, then it MUST abort the
   handshake with a "decode_error" alert.

second, which part of the RFC states that arbitrary data can be inserted at the end of a constructed type? how is it consistent with the rest of the RFC?

third, RFC 5246 Section 7.4.1.2 is quite explicit in stating that this kind of following data is forbidden:

   A server MUST accept ClientHello
   messages both with and without the extensions field, and (as for all
   other messages) it MUST check that the amount of data in the message
   precisely matches one of these formats; if not, then it MUST send a
   fatal "decode_error" alert.

and as you've rightfully pointed out, at this point in the code, we don't know which version of TLS we are negotiating, but rejecting both TLS 1.2 and TLS 1.3 ClientHello such formatted with decode_error is valid with both RFCs, rejecting it with illegal_parameter is not

[kaduk]

@kaduk
first: this is just a fragment of that paragraph, there is a second "if" just before the quoted sentence, with situation that applies to TLS 1.3, the "if not" applies when neither is the case:

I'm pretty sure the "if not" means "not [contains no data after legacy_compression_methods or contains valid extensions block]"; that's the more local binding, grammatically. That is, still conditional on negotiating 1.2.

second, which part of the RFC states that arbitrary data can be inserted at the end of a constructed type? how is it consistent with the rest of the RFC?

Nothing says that such arbitrary data can be inserted. The point is that we don't even have to check if we already know that the packet is not something we can handle, before we start parsing it.

and as you've rightfully pointed out, at this point in the code, we don't know which version of TLS we are negotiating, but rejecting both TLS 1.2 and TLS 1.3 ClientHello such formatted with decode_error is valid with both RFCs, rejecting it with illegal_parameter is not

We don't know what version of TLS we're negotiating, but we do know that it's not something worth spending our processing time on. If we were considering negotiating any version of TLS at this point, then these RFC directives would apply, but we have already decided that we are not negotiating anything, and rejecting the connection. Would you be happier if we decreed that "the other end is clearly talking nonsense" and sent a TCP RST? It really feels like you're tilting at a windmill, here.

[tomato42]

@kaduk
second, which part of the RFC states that arbitrary data can be inserted at the end of a constructed type? how is it consistent with the rest of the RFC?

Nothing says that such arbitrary data can be inserted. The point is that we don't even have to check if we already know that the packet is not something we can handle, before we start parsing it.

exactly, just by looking at the length of the message, you can tell that its encoding is invalid, in TLS 1.0, TLS 1.1, TLS 1.2 and in TLS 1.3. This change makes sure that it is rejected with an alert description prescribed by the RFC 5246 (partially by 8446) and not contradicted by any other RFC.

Would you be happier if we decreed that "the other end is clearly talking nonsense" and sent a TCP RST?

No, I expect an implementation to be consistent in the way it handles errors. If it sends Alert messages, it should send Alert messages always.

It really feels like you're tilting at a windmill, here.

Well, I consider this issue to be as clear-cut as they can be in TLS RFCs, and the change to address it, I think we can all agree, is quite trivial.

If we can't agree on such obvious things, I'm second guessing myself if I should be trying to contribute to OpenSSL in the first place. I don't expect OpenSSL developers to work on fixes to make testing of OpenSSL easier to other people, but if I am proposing changes to make testing of OpenSSL easier, changes that don't impact anybody else, and they get this kind of treatment, I see it as OpenSSL not being interested in 3rd party testing of their code.

[mspncp]

@t8m since more than a month has passed since your review, I didn't dare to upgrade your review retroactively to an OTC review. Please reconfirm.

[t8m]

I would like to see another OTC member review. I still think this change won't break anything, on the other hand from the discussion it is obvious it is a controversial change.

[t-j-h]

I have now raised the OTC vote and will report back on the status of the vote result.

[tomato42]

I am inclined to approve this - but we have a series of diverging views expressed so I am placing a -1 hold on this and will call an OTC vote to get a decision made. There are good arguments, well made, in both directions in my view and that is why we have a voting mechanism for resolving such issues.

to summarise my position for the vote:

• the RFCs define specific alert description when the ClientHello is malformed - the decode_error - currently this is not followed by OpenSSL in all cases
• the RFCs do not define specific alert description when other messages are malformed, thus changing the returned alert from illegal_parameter to decode_error does not cause OpenSSL to be RFC non-compliant

[t-j-h]

The vote to approve this PR did not pass - i.e. the OTC did not vote to accept this PR.

I suggested during the OTC discussion that we really should ask the IETF for a new alert to handle this situation as none of the existing alerts are actually a good match for this (their descriptions really don't align well with this context).

I'd appreciate other views on the topic of asking for a new alert (we could hold an OTC vote to make this request on behalf of the project itself) ...

[paulidale]

Asking the IETF for a new alert seems reasonable.

[richsalz]

You want error messages to be as generic as possible, otherwise you get into oracles. You can ask, but the TLS WG will not give one (in my opionion).

[kaduk]

It might be more prudent to try to get a new alert that wholly supersedes both illegal_parameter and decode_error and also explicitly covers the case here, as the distinction ends up not being terribly useful in practice (as we have seen here).

[davidben]

Or perhaps we just fold one into the other.

[richsalz]

Agree with #8300 (comment).

The question is the timetable for getting such an RFC out. Certainly at least a year.

[kaduk]

It's not entirely clear it would have to take that long (though if it stayed in my review queue as FIFO it probably would ☹️ ) The unavoidable delays are something like: 8 weeks at RFC production center, 2 weeks IESG evaluation, 2 weeks IETF LC, and 2-ish weeks WGLC, which leaves a bit of slop time to hit a 6-month target, assuming that there's not too much controversy in the WG and everyone involved has energy to keep things moving.

[richsalz]

Add 2-4 weeks to write the doc, 4-6 weeks for WG email review. Add time for crossing the Christmas/holdiay barrier.

[tomato42]

with such overhead I'd prefer to handle all the dark corners in a single RFC than to do it piecemeal

#8300 (comment)
You want error messages to be as generic as possible, otherwise you get into oracles. You can ask, but the TLS WG will not give one (in my opionion).

if an oracle though different error values is possible, then it definitely has a timing component and as such should get same kind of treatment as CBC mode got. Paving over it with a single error message only hides the issue, it doesn't fix it.

[richsalz]

if an oracle though different error values is possible, then it definitely has a timing component

No. Consider login which always asks for name/password but returns "no such user" or "wrong password" Nowadays they always return "Bad login"

[tomato42]

if an oracle though different error values is possible, then it definitely has a timing component

No. Consider login which always asks for name/password but returns "no such user" or "wrong password" Nowadays they always return "Bad login"

huh? If you return "bad login" after 1s when login is wrong but after 5s when the login is correct but password is wrong, then you have a timing oracle for user enumeration

[richsalz]

Yes, and if you have a bad username, but ask for a password and encrypt anyway there's no timing.

[tomato42]

Yes, and if you have a bad username, but ask for a password and encrypt anyway there's no timing.

oh, and the encrypted value is compared to to what? what if the server uses LDAP to retrieve those encrypted values? but I digress

if the error values for different kinds of errors must be the same because otherwise they will leak data, the specification needs to be very explicit about it and point out side-channel attacks — which means that testing and programming that part will be hard and error prone

but if the specification says that the different errors have to have different error messages, and many people looked at it and concluded that it doesn't leak any information, that means that the timing of those responses is irrelevant, making programming and testing of that code easier

collecting different errors into one error code "just in case" they may turn into an oracle won't make programmers write side-channel secure code "just in case" too

[richsalz]

but if the specification says that the different errors have to have different error messages, and many people looked at it and concluded that it doesn't leak any information,

I doubt this is true. Shrug.

[tomato42]

but if the specification says that the different errors have to have different error messages, and many people looked at it and concluded that it doesn't leak any information,

I doubt this is true. Shrug.

the original Bleichenbacher paper from 1998 depends on alert descriptions, not their timing

you really think that few other people looked at TLS specifications from that point of view since then?

[richsalz]

you really think that few other people looked at TLS specifications from that point of view since then?

for all error/alerts? and able to look at implementations? yes, I do think that.

[tomato42]

you really think that few other people looked at TLS specifications from that point of view since then?

for all error/alerts?

It really doesn't take a lot of effort to read 100 pages of protocol specification with a specific set of mind. I'm sure it's far less than turning specification into a model that can be algorithmically proven.

and able to look at implementations? yes, I do think that.

there's a difference between an implementation bug and a specification bug; sure, implementation doesn't need to follow specification to the letter, but specification really needs to provide "known good" behaviour if we want to have even a remote chance of seeing correctly behaving implementations

[vdukhovni]

This discussion has gone way off topic. Let's drop the side-channel subtopic, it is not pertinent here.

[kroeckx]

Both timing and the error code can be an oracle. But as long as it's based on public information, the only thing it leaks is which implementation it is. I'm not concerned about which error the RFC says we should sent in this case. Sending the other error code does not help the user to fix the error, or get a better error messsage.

[tomato42]

I'm not concerned about which error the RFC says we should sent in this case.

then why you're opposed to changing it?

Sending the other error code does not help the user to fix the error, or get a better error messsage.

why do you think it's unhelpful to send an alert indicating that the message is not well-formed upon receipt of a not well-formed message?

[openssl-machine]

Automated Ping: This issue is in a state where it requires action by @openssl/otc but the last update was 30 days ago

[openssl-machine]

This issue is in a state where it requires action by @openssl/otc but the last update was 60 days ago

[openssl-machine]

This issue is in a state where it requires action by @openssl/otc but the last update was 91 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 122 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 153 days ago

[mattcaswell]

The vote to approve this PR did not pass - i.e. the OTC did not vote to accept this PR.

Closing this PR due to the above decision.