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.

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

cmake: mention 'insecure' in the debug build warning #16327

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

No description provided.

@github-actions github-actions bot added the build label Feb 13, 2025
@vszakats vszakats closed this in 5d194d9 Feb 14, 2025
@vszakats vszakats deleted the cm-debug-insecure branch February 14, 2025 09:39
@jay
Copy link
Member

jay commented Feb 15, 2025

What's your rationale for this? It is quite enough to say do not use in production. A debug build is not itself insecure.

@vszakats
Copy link
Member Author

It depends on the debug build. If it means to contain debug symbols, asserts and -O0, it may still be secure, but this warning is for DEBUGBUILD builds which does more than that which also reduces security. This is OK, but felt it might need more emphasis.

@jay
Copy link
Member

jay commented Feb 15, 2025

this warning is for DEBUGBUILD builds which does more than that which also reduces security.

DEBUGBUILD is what I'm talking about and I think the point is that it is not inherently insecure so we shouldn't represent it that way. There are environment variables like CURL_ENTROPY that can lessen the security and some sanity checks are skipped like enforcing https for DoH. But that is not insecure. I went through the code and the only possible insecure code I see is in curl_msh3 #16342

@vszakats
Copy link
Member Author

vszakats commented Feb 15, 2025

I don't agree here. Overriding the random generator and disabling / overriding internals via envs doesn't seem secure to me. The feature is meant for internal testing (according to docs) where this is perfectly fine, but secure it isn't. The env list documented seems incomplete. It's not necessary just envs, but other behavior changes not documented, nor vetted from a security angle. Does the CVE program cover DEBUGBUILD builds?

For step-by-step debugging, getting stack dumps with symbols, DEBUGBUILD is not required. I suspect most users would expect these when enabling a "debug build" for a project. Perhaps the name DEBUGBUILD and ENABLE_DEBUG are wrong? Or perhaps I'm misundestanding something?

edit: Skipping HTTPS and DoH enforcement don't sound secure to me, though probably each should be double-checked case-by-case to know for sure.

@jay
Copy link
Member

jay commented Feb 15, 2025

I think we understand each other fine we just disagree

jay added a commit that referenced this pull request Feb 15, 2025
- Remove the workaround that disabled peer verification in DEBUGBUILDs
  when CA certs were provided.

The workaround was part of a TODO that disabled verification in
DEBUGBUILDs with a CAfile/path because apparently there's no way to set
those options in msh3 and that caused some tests to fail. Instead the
tests should fail and this problem should not be covered up.

Ref: #16327 (comment)

Closes #16342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants