-
Notifications
You must be signed in to change notification settings - Fork 60
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
Lower SECLEVEL in old
config for OpenSSL 3.x compatibility
#256
base: master
Are you sure you want to change the base?
Conversation
For DH parameters code review, I checked all servers which had .hbs configs containing lighttpd - no change; does the right things with the appropriate versions apache - small changes to do the right things with appropriate versions coturn - no change; does not call SSL_CTX_set_dh_auto/SSL_set_dh_auto There are some apps, such as stunnel and postgresql, which use built-in 2048-bit pregenerated DH params, if none is otherwise configured. Some other servers many support DH params included in certificate PEM file. This PR does not review bit sizes of DH params as that is out of scope for this PR -- which is focused on upgrading the OpenSSL version supported by ssl-config-generator. Besides, best practices in modern configs no longer use DHE. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
src/js/state.js
Outdated
if (configs[server].usesOpenssl !== false) { | ||
// set SECLEVEL=0 for openssl 3 to support TLSv1 and TLSv1.1 for Old config | ||
if (minver('3.0.0', form['openssl'].value) === true) { | ||
ciphers.unshift('@SECLEVEL=0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked into properly, only quick observation: https://gst-os3--mozsslconf-dev.netlify.app/#server=oraclehttp&version=12.2.1&config=old&openssl=3.3.2&guideline=5.7
# old configuration
SSLProtocol All
SSLCipherSuite @SECLEVEL=0:@SECLEVEL=0:@SECLEVEL=0:@SECLEVEL=0:@SECLEVEL=0:@SECLEVEL=0:@SECLEVEL=0:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-
Not that it wouldn't work, right? (I haven't tried tbh;D) but it just doesn't look right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is funny. I believe I had tested with stunnel.hbs, and hadn't looked across the rest. I'll see about modifying the javascript tomorrow or later in the weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a change to fix this. If you're concerned with the condition if (ciphers[0] !== '@SECLEVEL=0') {
on a possibly empty ciphers list (which should not happen for the Old config), then I can change the logic to something similar used to set usesDhe
further below in the same routine: if (ciphers.join(":").includes("@SECLEVEL=0") === false) {
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an urgent concern of mine, but the repetition that I now have to check for suggests that there are some performance gains on the client-side to be had by eliminating unnecessary repetition of work. Obviously, things need to be re-evaluated, when a config change is made by the user, but it should not need to be re-evaluated 5 times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was basically more concerned about how it got iterated over so many times in the first place;)
I was trying to avoid creating a new variable for @SECLEVEL=0 that had to be pasted onto the cipher list and have code added to each and every hbs
Sure, that is similar to tls13 logic also filtering out stuff in state.js before passing it on opaquely to the template output. That's definitely a good place!
(BTW, it's plain JS here, not HBS, so you don't have to nest the conditions so bizarrely (:) and can use falsy bool values without too much remorse really…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a javascript developer, so I appreciate any recommendations for better coding idioms or coding standard preferences.
Please add to your wishlist an issue to ask why this code is being run repeatedly for some templates. It did not appear to run repeatedly for the (new) stunnel template.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
eded910
to
0505bcc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
962806a
to
466d00d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
I will also try to document what info should be added upstream, to the recommendation contents, as a rationale for the choices we're making to support OpenSSL 3. (I will add it to the #260 epic where I intend to track things even spanning the actual repos…) I'm thinking about a review meeting to go through some of the things, TBD, I'll put together a list/backlog for that to see how much we have left that I'd like to discuss before confirming any decisions. (I have more questions than actual time to even ask them — some of the issues are solving themselves over time, I'm not sure about Tomcat impact (if you can try to find out it won't trip over the seclevel in cipherstring as it does some own parsing before passing anything on…), will have more questions about the seclevel bit-strength threshold wrt our other output values than just the protocols etc. …) What's mostly unknown to me are the os-level specifics that can differ for folks, e.g.:
Can you think of any wild differences among versions or distros/defaults? I would love for this to be first released as "opt-in" — say by skipping the 3.x version in configs.js now, we can ask people to manually test 3.x configs by punching in the version (or via permalink), before settling on that as the new default for anyone and everyone. Mainly to verify we're still good on 1.1.1 not bringing any regressions to the existing logic, and giving it few weeks for feedback before flipping that switch. (If it is long overdue, long+14d overdue doesn't make much difference.) |
No impact. Tomcat is configured in src/js/config.js with
From a practical perspective, it does not matter since it is required to support TLSv1 and TLSv1.1, and if the Old config provides support for TLSv1 and TLSv1.1, then
Ok. I will push momentarily with that commit removed. |
Ah right, I meant <112 bits, sorry. That kinda enforces the RSA and DHE keys length 2048b+ for us, so I'm not sure there's anything to review as that's on par with the recommendations (and good it's actually enforced now) — did you mean anything beyond that? |
My earlier comment was addressing the poster's recommendation to use at least ffdhe3072. That is not in scope for this PR and will be addressed in the future with specification review. :) |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
I wrote:
I should note that some servers hard-code ffdhe2048. stunnel is one. I think postgresql was another. (I was not keeping track as I quickly reviewed source code for calls to lighttpd is a third, but lighttpd only uses ffdhe2048 if openssl version is too old. The reason I have not updated lighttpd is because if you're on an ancient system and using an ancient openssl version, you are probably on an underpowered system and you likely have many other system security risks more important than DHE bit length. If this is a concern, you should, of course, configure lighttpd with your own appropriate |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
2048-bit is still plenty, using the hashrate of the bitcoin global network in 2024 I did some napkin math (see near end of comment for 2048-bit math) which calculated 54,000 years, but that was just against the entropy with the hashrate as operations. I don't have math on me from old notes for GFNS which also requires considerable resources beyond CPU at this scale, but IIRC the attack cost should be higher... thus really not pragmatic / worthwhile. TLS 1.3 also supports ffdhe2048 as a minimum IIRC and that's auto-negotiated? Usually you only see a desire to use more because the user does not understand the math, or some regulation compliance has raised the bar without adequate justification (presumably as a precaution or external perception/expectation). |
Rebased on master. After discussion in meeting, I have removed the dh_auto changes from this PR. (Note, this also removed the update to Apache version). Removing the dh_auto changes from this PR preserve the existing behavior for Intermediate to explicitly set 2048-bit DHE key. TLSv1.3 uses RFC7919 negotiation to select Group, and whether or not and how to configure Groups (Curves) will be a separate discussion from this PR. In the ssl-config-generator, The remaining commit in this PR sets |
src/js/state.js
Outdated
if (configs[server].usesOpenssl && minver('3.0.0', form['openssl'].value)) { | ||
// set SECLEVEL=0 via cipher string to support TLSv1-1.1 "old" with OpenSSL 3.x | ||
if (protocols.includes('TLSv1.1')) { | ||
if (!ciphers.includes('@SECLEVEL=0')) ciphers.unshift('@SECLEVEL=0'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at line lengths around (as L34) with similar conditionals, there's no formatting enforced really, but the order of evaluation mostly preferred, so I went for speed and simplification and e.g. this one stops evaluating once the leftmost is not relevant, so the protocol should go first etc.:
if (configs[server].usesOpenssl && minver('3.0.0', form['openssl'].value)) { | |
// set SECLEVEL=0 via cipher string to support TLSv1-1.1 "old" with OpenSSL 3.x | |
if (protocols.includes('TLSv1.1')) { | |
if (!ciphers.includes('@SECLEVEL=0')) ciphers.unshift('@SECLEVEL=0'); | |
} | |
} | |
if (protocols.includes('TLSv1.1') && configs[server].usesOpenssl && minver('3.0.0', form['openssl'].value)) { | |
// to allow "old" TLSv1-1.1 with OpenSSL 3.x we add SECLEVEL=0 via cipher string | |
if (!ciphers.includes('@SECLEVEL=0') && ciphers.length) ciphers.unshift('@SECLEVEL=0'); | |
} |
Also checking that we're actually adding it to an existing list, not creating a bogus list from empty result (there may not necessarily be an overlap of the guideline and the actual server able to offer own list of ciphers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, with the defaults not set in configs, the usesOpenssl value is not truthy, it's just unset, so needs precise testing… sorry.
if (configs[server].usesOpenssl && minver('3.0.0', form['openssl'].value)) { | |
// set SECLEVEL=0 via cipher string to support TLSv1-1.1 "old" with OpenSSL 3.x | |
if (protocols.includes('TLSv1.1')) { | |
if (!ciphers.includes('@SECLEVEL=0')) ciphers.unshift('@SECLEVEL=0'); | |
} | |
} | |
if (protocols.includes('TLSv1.1') && configs[server].usesOpenssl !== false && minver('3.0.0', form['openssl'].value)) { | |
// to allow "old" TLSv1-1.1 with OpenSSL 3.x we add SECLEVEL=0 via cipher string | |
if (!ciphers.includes('@SECLEVEL=0') && ciphers.length) ciphers.unshift('@SECLEVEL=0'); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, with the defaults not set in configs, the usesOpenssl value is not truthy, it's just unset, so needs precise testing… sorry.
Ok. Then your changes should have been minimal changes to address that, not rewriting other parts (yet again). Please try to be much, MUCH, more precise in making the minimum necessary changes.
I wrote the conditional logic in a specific order for a reason. The check for TLSv1.1 in protocols is only necessary for openssl 3.x where we have to change the security level. You checking for TLSv1.1 in protocols first "because you wanted to" is not the logical progression that I communicated in the way that I coded it or the intention of this PR, which is titled "Updates to support OpenSSL 3.x". Your change to reorder the logic was unnecessary and poor code review hygiene, besides a waste of time for all involved. Please try to get things done more expediently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review suggestion was not addressed so I went on to make the changes myself to finish testing and be able to release this.
If OpenSSL 3.x is gonna be the default, it will always be that case, so doesn't make sense to test first, the earliest to bail is to test for protocols, don't understand the rest of your concerns (whether they are real or just hypothetical, you'd have to be more specific, sorry.)
If you don't like my reviews, don't request them. Thanks. It was not in a working state i.e. not even tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasoning for order was in the first comment of this thread above the suggestion. Not "because I wanted to".
Reasoning for not creating just "@SECLEVEL=0" cipherstring from an empty list due to possible configs[server].supportedCiphers
intersection was described in the first post below the code suggestion.
Your rebase removing these, on the other hand, doesn't seem to be explained anywhere; so I'll just leave it to the others — as indeed, going back for the third time un-solving the comments at any cost really is a waste of everyone's time.
src/js/state.js
Outdated
} else { | ||
ciphers = ciphers; | ||
} | ||
if (protocols.includes('TLSv1.1') && configs[server].usesOpenssl !== false && minver('3.0.0', form['openssl'].value)) { | ||
// to allow "old" TLSv1-1.1 with OpenSSL 3.x we add SECLEVEL=0 via cipher string | ||
if (!ciphers.includes('@SECLEVEL=0') && ciphers.length) ciphers.unshift('@SECLEVEL=0'); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And shift when switching states:
} else { | |
ciphers = ciphers; | |
} | |
if (protocols.includes('TLSv1.1') && configs[server].usesOpenssl !== false && minver('3.0.0', form['openssl'].value)) { | |
// to allow "old" TLSv1-1.1 with OpenSSL 3.x we add SECLEVEL=0 via cipher string | |
if (!ciphers.includes('@SECLEVEL=0') && ciphers.length) ciphers.unshift('@SECLEVEL=0'); | |
} | |
} | |
if (protocols.includes('TLSv1.1') && configs[server].usesOpenssl !== false && minver('3.0.0', form['openssl'].value)) { | |
// to allow "old" TLSv1-1.1 with OpenSSL 3.x we add SECLEVEL=0 via cipher string | |
if (!ciphers.includes('@SECLEVEL=0') && ciphers.length) ciphers.unshift('@SECLEVEL=0'); | |
} else { | |
if (ciphers[0] === '@SECLEVEL=0') ciphers.shift(); | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done unconditionally before the logic which might add it, and also needs to check ciphers.length, since ciphers.length might be 0, e.g. with the current config for Modern with TLSv1.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- No need as it's either–or, no other case so if it's there it's tested and not added again; no need to remove beforehand, and only need to remove if it's there and shouldn't (not touching it otherwise on other state changes)
- I originally had
.at(0)
to not have worry about indexes/prototype etc.; but that doesn't have good browser support — need to check if that's somethingbabel
takes care of for us. - Not sure if happy trailing
ciphers?.[0]
evaluates better. - Basically no matter what type the var is, accessing the index even for empty array or a number, string or bool ends up falsy (no error is raised if that's what you're concerned about), the shift only happens when it's safe to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW instead of shifting an already manipulated cipher list, it'd be better to find why it's not being populated fresh on every state change from ssc.ciphers.openssl
(that it looks like it should, it just keeps referencing the already changed version from the past state(?))
4ca5757
to
d01b687
Compare
I have incorporated the two changes: 1) test |
github: closes mozilla#188
d01b687
to
8e1593c
Compare
Which doesn't change whether folks end up using OpenSSL or JSSE. I don't have enough working knowledge to infer how the connectors configure own
Because it's the most sensitive setting in the current outputs, and of the added value features we provide translating the guidelines into the configs.
Which is not true. All TLSv1.3 suites include them by default (even though they don't "advertise" the fact in the IANA naming, but it's defined directly in the RFC). |
@polarathene Ah, that came from someone who got bad rating from an automated test unrelated to these configs, it was grading against Dutch Internet Standards Platform, but that probably evaluates the government recommendations wrong anyways, as only 1024 should fail, 2048 is still allowed even after years of updates to their specs, although noted to "phase out" — but they have no instructions attached as to what that is supposed to mean, i.e. it's still listed nonetheless. (I'd see the requirement for 3072 more as a bug in the automation tool TBH.) We still reflect what is in the current recommendations, and will try to do so even for RFC7919 groups, if we end up restricting them in the configs. (See #270 …) |
old
config for OpenSSL 3.x compatibility
Yeah, I mentioned some reasons for why someone might feel the need to use a higher key/group size but as per the napkin math, I really don't see it being necessary. Given that TLS 1.3 will handle many connections these days and IIRC a client or server may restrict it's ability to only negotiate with a cipher using FFDHE 2048-bit, since TLS 1.3 doesn't allow you to exclude that (not since I last checked anyway?) it'd be accepted 🤔 Since it's a static group for any such TLS 1.3 connection negotiated with it, someone with the resources could attack that for an advantage. At which point some policy goes into place to forbid/change it, but as per the math I linked earlier it's not very pragmatic to pull that off:
The link provides more context/info for that, but you need an awfully large amount of computation and advancement to bring that attack into the realm of possibility, as covered in another part of my napkin math reference:
The power required is sufficient to boil all the oceans on the planet, it's not something you can do stealthily even if we could compute an attack within our lifetimes. Unfortunately we have silly articles like this 2024 Forbes one encouraging 3072-bit and citing quantum compute risks:
Which to anyone who knows better this is nonsense. We are a long way off having the necessary qubits (the hardware you see marketed these days tends to be a lot lower in actual qubits since many are used for error correction). While the compute time could go down, it doesn't magically mess with physics, the entropy is still there and the resource cost just shifts, time is only one resource to take into consideration.
RSA keys, not FFDHE group parameters but close enough compute wise IIRC 😅 That's the sort of thing that'll make a push for bumping it up, regardless of the math. That increase is smaller than RSA 1024-bit to 2048-bit too (NIST has their own document on equivalent symmetric security strength). |
Updates to support OpenSSL 3.x
Resolves #188
Tracked in #260
old
output to allow TLSv1–v1.1 for OpenSSL 3.xHide DH parameters for servers calling OpenSSL interfaces(TLS1.2 will keep just ffdhe2048, TLS1.3 will have to limit curves/groups, followup TBD)SSL_CTX_set_dh_auto()
orSSL_set_dh_auto()
to use RFC7919.TODO/OQ:
consider AND helper for conditional blocks only (not) matching two valuesTESTING:
https://test-3x--mozsslconf-dev.netlify.app/#openssl=3.3.3&config=old&server=postgresql