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

Errors in config file values get swallowed, disable other directives #20789

Open
t184256 opened this issue Apr 20, 2023 · 5 comments
Open

Errors in config file values get swallowed, disable other directives #20789

t184256 opened this issue Apr 20, 2023 · 5 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 triaged: bug The issue/pr is/fixes a bug

Comments

@t184256
Copy link
Contributor

t184256 commented Apr 20, 2023

OpenSSL master (c809334, ./config no-shared).

I'm gonna define the following config
for use with OPENSSL_CONF=openssl.cnf apps/openssl s_client ecc256.badssl.com:443
and vary just the last two lines for brewity.

config_diagnostics = 1
openssl_conf = openssl_init

[openssl_init]
providers = provider_sect
ssl_conf = ssl_module

[provider_sect]

[ssl_module]
system_default = policy

[policy]
SignatureAlgorithms = ECDSA+SHA256
#Groups = P-521

That allows me to establish a TLS 1.2 connection using ECDSA-SHA256 and prime256v1.

Uncommenting Groups:

SignatureAlgorithms = ECDSA+SHA256
Groups = P-521

leads to SSL alert number 40, which I'm OK with.

Now let's add an unrecognized value to Groups:

SignatureAlgorithms = ECDSA+SHA256
Groups = P-521:nonex

That results in a successful connection like in a baseline case,
so I'm assuming Groups reverts to whatever default it has.

What's worse, is that this effect works across lines:

SignatureAlgorithms = ECDSA+SHA256:nonex
Groups = P-521

also results in a successful connection, as if Groups is ignored!

Finally, switching the order of the directives

Groups = P-521
SignatureAlgorithms = ECDSA+SHA256:nonex

makes the masking effect go away (connection fails),
so I assume it's the rest of the config (section?) that gets ignored
after encountering one invalid value.

I'm aware of config_diagnostics = 1, but it doesn't seem to help here.

Please consider failing hard on encountering an invalid/unrecognized value.
Not only the current behaviour is painfully error prone,
(Groups = P-384:P-512 is the same as Groups = P-384,P-521)
but also the scope of the resulting misconfiguration is much wider than my wildest imagination
(they both effectively mean "ignore this and following lines", somehow).
As a result, it becomes very hard to configure the algorithm selection in a robust and secure manner.

@t184256 t184256 added the issue: bug report The issue was opened to report a bug label Apr 20, 2023
@mattcaswell mattcaswell added triaged: bug The issue/pr is/fixes a bug and removed issue: bug report The issue was opened to report a bug labels Apr 20, 2023
@mattcaswell
Copy link
Member

What's worse, is that this effect works across lines:

It should not have an impact across lines

SignatureAlgorithms = ECDSA+SHA256:nonex

My assumption is that the default sigalgs are being used and some non-ec based cipher suite is negotiated meaning that the "Groups" config value is not relevant.

@t184256
Copy link
Contributor Author

t184256 commented Apr 20, 2023

My assumption is that the default sigalgs are being used and some non-ec based cipher suite is negotiated meaning that the "Groups" config value is not relevant.

That'd be great, but the last example (reordering of lines changing the outcome) seems to refute it.

@mattcaswell
Copy link
Member

That'd be great, but the last example (reordering of lines changing the outcome) seems to refute it.

Hmm. Good point. That does seem odd. There shouldn't be any across line effects...requires further investigation as to what is happening here.

@t8m
Copy link
Member

t8m commented Apr 20, 2023

IMO the right thing would be to behave similarly to ciphers parsing, i.e. ignore unknown groups or sigalgs.

So

Groups = P-521:nonex

and

Groups = P-521

should behave the same.

@beldmit
Copy link
Member

beldmit commented Apr 20, 2023

Or emit warning/error in strict mode.

@beldmit beldmit added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

No branches or pull requests

4 participants