Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Additional TLS checks #7029

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Additional TLS checks #7029

merged 2 commits into from
Dec 13, 2023

Conversation

julek-wolfssl
Copy link
Member

  • 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
Contributor

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 requested changes Dec 11, 2023
View reviewed changes
Copy link
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
Member Author

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
src/internal.c Outdated Show resolved Hide resolved
src/internal.c Outdated Show resolved Hide resolved
@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
Member Author

Retest this please.

dgarske
dgarske requested changes Dec 12, 2023
View reviewed changes
Copy link
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
Member Author

@dgarske build option added with WOLFSSL_DISABLE_EARLY_SANITY_CHECKS.

dgarske
dgarske approved these changes Dec 12, 2023
View reviewed changes
Copy link
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
Contributor

retest this please

douzzer
douzzer approved these changes Dec 13, 2023
View reviewed changes
Copy link
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
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
108 checks passed
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
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