Skip to content

harden: malformed-input hardening, non-panicking encoders, unit coverage - #443

Open
MattJackson wants to merge 8 commits into
stack/s4from
stack/s5
Open

harden: malformed-input hardening, non-panicking encoders, unit coverage#443
MattJackson wants to merge 8 commits into
stack/s4from
stack/s5

Conversation

@MattJackson

@MattJackson MattJackson commented Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Robustness hardening and broad unit-test coverage across the codec/token layer.

  • Bounds/robustness hardening across codec and token decoders against malformed server input.
  • Non-panicking var-len encoders; DONE-status leniency.
  • AAD-token redaction/zeroize; numeric rescale to column scale instead of panicking.
  • Broad in-crate unit-test coverage across decoders, time, tokens, config.

Squashed into a single commit (all the submitter's own work — no external authorship to preserve here).

Supersedes: none.

Sequential series — merge after #432–#435. Based on main; diff reduces to its own delta as the earlier PRs land.
Reviewer note: rebase-merge or merge-commit, not squash (keeps the series consistent).

@MattJackson MattJackson changed the title stack/s5 harden: malformed-input hardening, non-panicking encoders, unit coverage Sep 2, 2026
@MattJackson

MattJackson commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

📌 Carrying over review feedback from the previous incarnation of this PR (#436), which is being closed in favour of a native stacked-PR chain.

@victorchutw raised a still-open correctness issue on this branch — Numeric::precision() counts a phantom integral digit when the integral part is zero (1 + scale), so a fraction-only scale-38 value reports numeric(39,38), which SQL Server rejects. Full report and a focused fix commit here: #436 (comment)

This is tracked and not yet addressed on this branch — flagging so it isn't lost in the migration. Thanks @victorchutw.

This was referenced Sep 2, 2026
Closed
Open
@MattJackson
MattJackson force-pushed the stack/s5 branch 2 times, most recently from 036a4da to a1854c6 Compare September 2, 2026 20:24
@MattJackson
MattJackson force-pushed the stack/s5 branch 3 times, most recently from 2b83ba6 to e9b8f48 Compare September 2, 2026 23:54
@MattJackson MattJackson closed this Sep 3, 2026
@MattJackson MattJackson reopened this Sep 3, 2026
@MattJackson

MattJackson commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@aqrln rebased onto the merged main, all green and MERGEABLE — ready for review whenever you get a chance. Thanks!

@MattJackson

MattJackson commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Updated + green. Replaced a todo!() in the VarLenSizedPrecision decode path (a remote-DoS panic on crafted server input) with a protocol error, switched numeric rescale to checked arithmetic, and turned two silent date/offset corruption paths into explicit errors (out-of-range day offset; out-of-range DATETIMEOFFSET offset).

Note on the encryption default (Off → Required): when an ADO.NET/JDBC connection string omits the encrypt keyword, the negotiated level is now Required instead of Off. This makes the connection-string parser consistent with Config::default() (always Required), matches upstream main (0.13.1), and matches modern ADO.NET's Encrypt=Mandatory. It fails closed rather than silently sending credentials in plaintext; explicit opt-out via encrypt=false (or encrypt=DANGER_PLAINTEXT).

@MattJackson
MattJackson force-pushed the stack/s5 branch 2 times, most recently from e94c4b1 to 1e748b4 Compare September 16, 2026 21:11
@MattJackson
harden: malformed-input hardening + non-panicking encoders + unit cov…
ac39aa8 
…erage across codec/tokens/config
@MattJackson
fix: correct false prelogin comment; drop mutation-test noise
45b5f96 
- pre_login: comment claimed unknown tokens are skipped, but they are
  rejected as a protocol error (matching the existing test) — fix the comment
- revert the 1<<0 -> 1 churn and drop the "shift-invariant" comments
- trim mutation-testing narration comments across codec/token modules
- remove "previously…" history comments (fixed_len, token_col_metadata, numeric)
- sql_read_bytes: use std::pin::pin! instead of unsafe Pin::new_unchecked in a test
@MattJackson
fix(codec): reject unexpected precision/scale type and overflowing nu…
d094de7 
…meric rescale

- column_data decode: replace todo!() for a non-Decimaln/Numericn
  VarLenSizedPrecision type with a Protocol error. ty is server-controlled,
  so todo!() was a remote-DoS panic. Convert the codifying #[should_panic]
  test to assert the returned Protocol error.
- numeric rescale on encode: target_scale is server-controlled (u8); the
  10i128.pow(delta) sites panicked in debug / silently wrapped in release.
  Use checked_pow + checked_mul and return a Protocol error on overflow.
  Add tests for scale-up factor overflow and multiply overflow.
@MattJackson
fix(codec): error instead of panic on BigChar/BigVarChar missing coll…
4215133 
…ation

Server-omitted collation made string decode unwrap() panic (remote DoS).
Return a Protocol error via ok_or_else and add a decode test.
@MattJackson
fix(time): reject out-of-range day offsets and datetimeoffset offsets
fcdf66f 
Server-controlled date/offset fields previously produced silently wrong
values instead of errors:
- from_days clamped out-of-range day offsets to MIN/MAX. Every valid SQL
  date fits within time::Date/NaiveDate, so this only masked malformed
  input; return a Protocol error instead of a plausible-but-wrong date.
- datetimeoffset offset fell back to UTC (time) / saturated the subtraction
  (chrono) for offsets outside SQL Server's valid -14:00..=+14:00 range,
  silently shifting the instant. Validate the offset and return a Protocol
  error.
- restructure the from_sql! arms from .map() closures to match blocks so the
  ? operator propagates errors to the FromSql/FromSqlOwned fn (no macro or
  wire-byte changes; in-range values decode identically).
- scale>9 in nanos_from_increments was already guarded by saturating
  arithmetic (no panic); add a regression test.

Update the from_days_clamps_on_overflow tests to assert the error and add
tests for out-of-range offsets on both the time and chrono paths.
@MattJackson
test: cover malformed SSRP replies and non-prelogin TLS handshake pac…
805d0d0 
…kets

Add coverage for already-correct hardening guards:
- sql_browser: truncated (<3 byte) SSRP UDP reply is rejected without
  panicking on the &buf[3..len] slice; well-formed reply resolves a port.
- tls: a non-PreLogin packet type and a prelogin length shorter than the
  8-byte header are rejected with InvalidData during the handshake, driven
  through poll_read with a mock stream (no logic change).
@MattJackson
fix(rebuild): dedupe Display money arm and meta helper; drop obsolete…
b103e52 
… debug-name test and orphaned const docs
@MattJackson
fix(connection): use renamed encode_to_boxed_slice on the AAD-token l…
7d05d40 
…ogin path

Propagates the s3 `encode_to_vec` -> `encode_to_boxed_slice` rename to the
second call site (added here for the sensitive AAD-token login path).
@MattJackson MattJackson mentioned this pull request Sep 18, 2026
Open
@MattJackson
MattJackson requested review from a team and aqrln September 18, 2026 00:38
This was referenced Sep 18, 2026
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aqrln aqrln Awaiting requested review from aqrln

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@MattJackson