Skip to content

Additional TLS checks#7029

Merged
SparkiDev merged 2 commits intowolfSSL:masterfrom
julek-wolfssl:zd/17108-fix
Dec 13, 2023
Merged

Additional TLS checks#7029
SparkiDev merged 2 commits intowolfSSL:masterfrom
julek-wolfssl:zd/17108-fix

Conversation

@julek-wolfssl
Copy link
Copy Markdown
Member

@julek-wolfssl julek-wolfssl commented Dec 5, 2023

  • double check which messages need to be encrypted
  • check msgs that have to be last in a record

ZD17108

@julek-wolfssl julek-wolfssl self-assigned this Dec 5, 2023
@julek-wolfssl julek-wolfssl force-pushed the zd/17108-fix branch 3 times, most recently from e35c9e9 to c070c01 Compare December 6, 2023 16:16
JacobBarthelmeh
JacobBarthelmeh previously approved these changes Dec 8, 2023
View reviewed changes
@JacobBarthelmeh
Copy link
Copy Markdown
Contributor

JacobBarthelmeh commented Dec 8, 2023

Looks good to me, would like at least one other review on this though since the code changes will affect all TLS/DTLS users.

@JacobBarthelmeh JacobBarthelmeh assigned rizlik and unassigned wolfSSL-Bot and rizlik Dec 8, 2023
@julek-wolfssl julek-wolfssl assigned SparkiDev and unassigned rizlik Dec 11, 2023
@dgarske dgarske added the For This Release Release version 5.9.1 label Dec 11, 2023
dgarske
dgarske requested changes Dec 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@dgarske dgarske left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR looks good. It grows my build by 384 bytes. Should we consider a way to disable this feature, but keep it on by default?
I'd like Sean to review before its merged.

@dgarske dgarske removed their assignment Dec 11, 2023
@julek-wolfssl
Copy link
Copy Markdown
Member Author

julek-wolfssl commented Dec 11, 2023

I don't think that its a good idea to remove these checks. These are important checks that are relevant for increasing security of all (D)TLS versions.

SparkiDev
SparkiDev requested changes Dec 11, 2023
View reviewed changes
Comment thread src/internal.c Outdated
Comment thread src/internal.c Outdated
@julek-wolfssl
Additional TLS checks
627310d 
- double check which messages need to be encrypted
- check msgs that have to be last in a record

ZD17108
@julek-wolfssl
Copy link
Copy Markdown
Member Author

julek-wolfssl commented Dec 12, 2023

Retest this please.

dgarske
dgarske requested changes Dec 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

PR looks great. Can you please add a build option to disable this check for users that might not want it?

@julek-wolfssl
Copy link
Copy Markdown
Member Author

julek-wolfssl commented Dec 12, 2023

@dgarske build option added with WOLFSSL_DISABLE_EARLY_SANITY_CHECKS.

dgarske
dgarske approved these changes Dec 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Over to @SparkiDev to finalize.

@dgarske dgarske assigned SparkiDev and unassigned dgarske Dec 12, 2023
@wolfSSL wolfSSL deleted a comment from dgarske Dec 12, 2023
@SparkiDev
Copy link
Copy Markdown
Contributor

SparkiDev commented Dec 12, 2023

retest this please

douzzer
douzzer approved these changes Dec 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

passed a run through wolfssl-multi-test.sh ... super-quick-check (latest sanitizers, clang-tidy, and cppcheck), rebased on current master.

@dgarske
Copy link
Copy Markdown
Contributor

dgarske commented Dec 13, 2023

Retest this please

@SparkiDev SparkiDev removed the request for review from rizlik December 13, 2023 04:30
@SparkiDev SparkiDev merged commit f12b611 into wolfSSL:master Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dgarske dgarske dgarske approved these changes

@douzzer douzzer douzzer approved these changes

@SparkiDev SparkiDev SparkiDev approved these changes

@JacobBarthelmeh JacobBarthelmeh JacobBarthelmeh left review comments

Assignees

@wolfSSL-Bot wolfSSL-Bot

@SparkiDev SparkiDev

Labels

For This Release Release version 5.9.1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@julek-wolfssl @JacobBarthelmeh @SparkiDev @dgarske @douzzer @rizlik @wolfSSL-Bot