Skip to content

Conversation

@SparkiDev
Copy link
Contributor

Description

In TLS 1.3, ignore valid unencrypted alerts that appear after encryption has started.
Only ignore WOLFSSL_ALERT_COUNT_MAX-1 alerts.

Fixes zd#20857

Testing

./configure --disable-shared
make
./tests/unit.test -test_tls13_plaintext_alert

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@SparkiDev SparkiDev self-assigned this Nov 24, 2025
@devin-ai-integration
Copy link
Contributor

🛟 Devin Lifeguard found 1 likely issues in this PR

  • check-all-return-codes snippet snippet snippet: Capture and assert the return value of wolfSSL_SetIORecv, wolfSSL_SetIOSend and wolfSSL_SetIOReadCtx (e.g. ExpectIntEQ(wolfSSL_SetIORecv(ctx, MbRecv), WOLFSSL_SUCCESS);).

@SparkiDev
please take a look at the above issues which Devin flagged. Devin will not fix these issues automatically.

@SparkiDev SparkiDev force-pushed the tls13_pt_alert_when_enc branch 2 times, most recently from 130c70d to 8943d6c Compare November 24, 2025 08:08
@rizlik
Copy link
Contributor

rizlik commented Nov 24, 2025

If the goal of this PR is to protect against DoS I don't think it's a good idea:

  • under TCP one can easily forge a RST packet and close the connection
  • peer can still forge arbitrary packets that cause the handshake to error out (handshake, app_data) ecc

UDP is different but DtlsShouldDrop ignores plaintext packet after handshake keys are computed.

@SparkiDev
Copy link
Contributor Author

SparkiDev commented Nov 24, 2025

DoS can be done by any message, it doesn't have to be a valid alert.
But yes that is the report.

Instead the PR is about skipping alerts that were sent by the client before it received anything from the server to indicate it should be encrypted. OpenSSL and others do this.

May make this a compile time option.

@SparkiDev
Copy link
Contributor Author

retest this please

dgarske
dgarske previously approved these changes Dec 4, 2025
Copy link
Member

@julek-wolfssl julek-wolfssl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't agree with fixing it. IMO ignoring plaintext alerts is going to introduce edge cases in the transition period between plaintext and ciphertext. This in turn can hang clients/servers that think the connection is still alive when its really dead. The argument that its a DoS of a connection doesn't make any sense since injecting garbage would also kill the connection.

@SparkiDev
Copy link
Contributor Author

Changed to be compile time option when WOLFSSL_TLS13_IGNORE_PT_ALERT_ON_ENC is defined.
Test is testing as many alerts as is required to cause an error or one less being OK.
Test is checking that without define that an error is returned.

In TLS 1.3, ignore valid unencrypted alerts that appear after encryption
has started.
Only ignore WOLFSSL_ALERT_COUNT_MAX-1 alerts.
@SparkiDev SparkiDev force-pushed the tls13_pt_alert_when_enc branch from 8a94314 to 9bce18f Compare December 10, 2025 02:14
@SparkiDev
Copy link
Contributor Author

retest this please

@SparkiDev SparkiDev removed their assignment Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants