Skip to content

Enable SCSV check unconditionally#10661

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_5808
Open

Enable SCSV check unconditionally#10661
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_5808

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Enable TLS_FALLBACK_SCSV check unconditionally

Summary

The TLS_FALLBACK_SCSV (RFC 7507) anti-downgrade enforcement was previously gated
behind the HAVE_FALLBACK_SCSV build option (or OPENSSL_ALL), so a default
build silently ignored a client's TLS_FALLBACK_SCSV (0x5600) signaling cipher.
This makes the check always compiled in and fixes a latent bug in how the
server determines whether a real downgrade occurred.

Changes

Always enforce the check (src/internal.c)

  • Removed the #if defined(HAVE_FALLBACK_SCSV) || defined(OPENSSL_ALL) guard
    around the TLS_FALLBACK_SCSV handling in DoClientHello; the check now runs
    in every build.
  • Dropped the HAVE_FALLBACK_SCSV option from the build system and docs
    (configure.ac, CMakeLists.txt, examples/configs/user_settings_all.h,
    wrapper/Ada/user_settings.h, and the option list comment in internal.c).

Fix the downgrade comparison

  • The old code compared the client-offered version against
    ssl->ctx->method->version.minor (the compiled maximum). That ignored
    runtime restrictions such as SSL_OP_NO_TLSv1_3 on a TLS 1.3-capable method,
    producing a false-positive inappropriate_fallback abort for a client that
    legitimately offered the server's effective maximum version.
  • Now snapshots the server's effective max (ssl->version.minor) into
    maxMinor before the downgrade logic lowers it to the negotiated version,
    and compares against that.
  • Corrected the comparison direction for DTLS, whose version minors decrease
    as the version increases (DTLS 1.0 = 0xff, 1.2 = 0xfd, 1.3 = 0xfc):
    if ((!ssl->options.dtls && maxMinor > pv.minor) ||
        ( ssl->options.dtls && maxMinor < pv.minor)) {

Tests (tests/api/test_tls.c, test_tls.h)

Added four targeted handshake tests driven through the manual-memio harness:

Test Purpose
test_tls_fallback_scsv TLS 1.2 ClientHello + SCSV vs. a TLS 1.3-max server → must abort VERSION_ERROR and send inappropriate_fallback (86) on the wire.
test_dtls_fallback_scsv DTLS 1.2 ClientHello + SCSV vs. a DTLS 1.3-max server → exercises the inverted DTLS comparison; checks the alert by offset to stay robust against DTLS epoch/seq bytes.
test_tls_fallback_scsv_no_downgrade Negative control: SCSV against a server whose max is the offered version → handshake must not be rejected for the fallback reason.
test_tls_fallback_scsv_no_downgrade_runtime_max Negative control for the runtime-max fix: TLS 1.3-capable method with TLS 1.3 disabled at runtime, client offers TLS 1.2 → must not false-positive (the exact case the old method->version.minor comparison broke).

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 11, 2026
Copilot AI review requested due to automatic review settings June 11, 2026 02:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR makes RFC 7507 TLS_FALLBACK_SCSV enforcement always compiled in (no build-time gate), fixes the server-side downgrade comparison to use the effective runtime max protocol version (including DTLS inversion rules), and adds targeted API tests to validate both positive and negative cases.

Changes:

  • Remove the HAVE_FALLBACK_SCSV build option and associated configuration/docs defines so fallback-SCSV handling is always present.
  • Fix downgrade detection by snapshotting the server’s effective max version (ssl->version.minor) before negotiation mutates it, and invert the comparison for DTLS.
  • Add four manual-memio handshake tests covering TLS/DTLS inappropriate_fallback behavior and two “no downgrade” negative controls (including runtime-disabled TLS 1.3).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/internal.c Always enforce TLS_FALLBACK_SCSV and correct downgrade comparison using a pre-negotiation max snapshot, including DTLS inversion.
configure.ac Remove the --enable-fallback-scsv option and related flag/summary output.
CMakeLists.txt Remove mention of the fallback SCSV option from the option list comment.
examples/configs/user_settings_all.h Drop HAVE_FALLBACK_SCSV define from the “all settings” example config.
wrapper/Ada/user_settings.h Drop HAVE_FALLBACK_SCSV define from Ada wrapper user settings.
tests/api/test_tls.h Register new fallback-SCSV tests in declarations and test list.
tests/api/test_tls.c Add TLS/DTLS fallback-SCSV tests (positive + negative controls) via manual-memio harness.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/api/test_tls.c Outdated
Comment thread tests/api/test_tls.c Outdated
Comment thread configure.ac
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

gcc-arm-cortex-m4

  • FLASH: .text +64 B (+0.0%, 198,958 B / 262,144 B, total: 76% used)

gcc-arm-cortex-m4-rsa-only

  • FLASH: .text +64 B (+0.0%, 323,000 B / 1,048,576 B, total: 31% used)

gcc-arm-cortex-m7

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #10661

Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src

No new issues found in the changed files. ✅

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.

4 participants