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

Can't detect multiple certificates #208

Open
agrandville opened this issue Jun 9, 2020 · 9 comments
Open

Can't detect multiple certificates #208

agrandville opened this issue Jun 9, 2020 · 9 comments

Comments

@agrandville
Copy link

For performance improvement some sites let client choose which signature algorithm they want to use (mainly RSA or ECDSA).

eg: www.google.com

openssl s_client -sigalgs RSA+SHA256 www.google.com:443 | openssl x509 -noout -text
....
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (2048 bit)
....

openssl s_client -sigalgs ECDSA+SHA256 www.google.com:443 | openssl x509 -noout -text
...
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
...

wouldn't it more accurate to show all certificates when --show-certificate(s) is requested ?

@rbsec
Copy link
Owner

rbsec commented Jun 9, 2020

Hi @agrandville,

This is an interesting idea - I've seen a few sites that have multiple certificates like this, so it would be nice to have some detection for that. However, we need to think about how we can display that (the certificate display is already very verbose, and returning multiple certificates is a breaking change to the XML).

In terms of scanning, I guess this would have to sit in the loop where we detect which signature algorithms the server supports, although this would be made more complicated by servers that claim to support arbitrary algorithms. So maybe it would have to be in a separate loop...

@jtesta what are your thoughts on this?

@jtesta
Copy link
Contributor

jtesta commented Jun 9, 2020 via email

@rbsec
Copy link
Owner

rbsec commented Jun 9, 2020

@jtesta one potential issue with using a list of accepted algorithms is that if the server accepts the bogus one, then not further testing is carried out for them - so we'd either always have to run the full test, or have a default list to try in that case.

If the XML change is going to happen then that should be before version 2.0 gets released properly, and hopefully it won't break too much stuff.

@jtesta
Copy link
Contributor

jtesta commented Jun 9, 2020 via email

@rbsec
Copy link
Owner

rbsec commented Jun 9, 2020

That's a fair point.

Actually, looking at the current code, you can already get multiple <certificate> blocks - you get one by default, and a second one with more details if you used --show-certificate. I wonder how many parsers that breaks...

That should probably be cleaned up, and the showCertificate() and checkCertificate() functions merged together into one.

@jtesta
Copy link
Contributor

jtesta commented Jun 9, 2020 via email

@rbsec
Copy link
Owner

rbsec commented Jun 9, 2020

To be honest, it's not a huge deal if we change the XML a bit for 2.0 (especially given it's a bit dodgy at the moment).

I'll look at adding the <certificates> element and merging the two check functions for now, and then we can look at actually doing the multiple certificate checks down the line.

@rbsec rbsec changed the title Can't detect multiple certificats Can't detect multiple certificates Jun 9, 2020
@joaociocca
Copy link

joaociocca commented Mar 1, 2023

is this what's causing this malformation of the XML file?

image

I noticed it while trying to work sslscan's XML file with xq and getting an xq: Error running jq: ExpatError: mismatched tag: line 13, column 4.

@joaociocca
Copy link

ok, I figured out a way to remove those empty </certificate> thingies, sharing here in case someone stumbles on this too:

sed -ibkp ':a;N;$!ba;s#\n <certificates>\(\n \+</certificate>\)\+\n </certificates>##g'  <file>

example:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants