Consider to add descriptive field in JSON

[aress31]

Hi,

I am working a lot with testssl prettyJSON outputs, actually I made a script to parse the output and automatically generate Excel spreadsheets from the prettyJSON outputs (see https://github.com/AresS31/testssl2xlsx).

When developing that script I had to handle a few things that I think should be handled by testssl.

The following are my improvement suggestions:

• Unique and more explicit ids for cipherTests objects. Instead of secure_renego use Secure Renegotiation for example, and all ids should be consistent, either all in lowercase or all in uppercase (even for acronyms). Also ids should not be too similar to each others because as you can see on the following screenshots, there are 2 objects with the same id of drown; logjam and LOGJAM_common primes both contain the string/sub-string logjam from a coding point of view this create the need to add additional checks and line of codes to ensure that the right object is being processed.
  
  

• Instead of using the value OK as severity when the host:port is not vulnerable instead use NOT VULNERABLE to add more clarity and avoid any confusion.

• For the finding field here too adding more consistency, see the following screenshot, for lucky13 the finding does not start with lucky13 whereas for rc4 it starts with RC4: not. So more consistency and maybe more verbose in the explanation, something that could directly be used as a remediation.
  

• For the protocols section the ids for the protocols should be more consistent as well like we have got sslv2, sslv3 but then tls1, tls1_1 why not replacing them by tlsv1.0, tlsv1.1, etc.

• Adding an additional field to the protocols section, it could be enabled: YES and enabled: NO or offered: YES and offered: NO so we could programmatically just test the value of this JSON field to determine if a protocol is being offered rather than working out if the finding field contains the string not offered.

This are my suggestions they are all relevant to my case but it is up to you if you judge them relevant for a wider use.

Great job!
Alex

[drwetter]

Thanks Alex for the lengthy description of suggestions. I am not sure I want to follow this by 100% but some suggestions make sense.

There's (little) overlap with #830. The convincing thing argument was the colon: There shouldn't be a need to add in finding what is already in id.

All of them are breaking changes though which doesn't leave much room changing if back and forth.

Detailed comments will follow later.

[aress31]

@drwetter thanks for you answer. I am looking forward to see what you will decide to do.

A last thing that I noticed and forgot to mention in my first post is that currently ALL the vulnerabilities tested by teslssl are listed in the prettyJSON output file when using the --vulnerable switch even if it is just to say OK this target is not vulnerable. However, for non-HTTP(S) services let's say RDP for example (port 3389), I noticed that the BEAST vulnerability (maybe other vulnerabilities too) is not included in cipherTests.

Is that a bug or just the expected behavior?

Regards,
Alex

[bao7uo]

Browser Exploit Against SSL/TLS

[aress31]

@bao7uo you missed my point.

My point was that even if not relevant to the service it should still be listed as not-vulnerable in my opinion, so there is a consitent number of entries within the cipherTests regardless of the nature of the service tested, for better parsing and automation.

[bao7uo]

no, i agree with you that consistency is better. i just thought that may explain the inconsistency, undesirable though it may be :)

[drwetter]

What I could think of is adding e.g. BEAST for services != HTTP and add N/A as a finding. Don't know yet whether I would like that.

There some occasions where the checks being run depend on the service detected, see client handshake != HTTP or SPDY and ALPN for non-HTTP services.

I consider this similar to nmap's scripting engine: If you supply -sC it doesn't run http-related NSE scripts on RDP.

[drwetter]

Unique and more explicit ids for cipherTests objects. Instead of secure_renego use Secure Renegotiation for example

I am thinking about it for the not-so-close-future but I am not sure I like the blanks or too long ids. Last but not least it's a machine readable format. I still believe though humans should be able to read it but probably that goes too far.

ids should be consistent, either all in lowercase or all in uppercase (even for acronyms)

Nah, acronyms have to be in caps. But if you put everything in caps it's like yelling

Also ids should not be too similar to each others because as you can see on the following screenshots, there are 2 objects with the same id of drown; logjam and LOGJAM_common primes both contain the string/sub-string logjam from a coding point of view this create the need to add additional checks and line of codes

Don't know what kind of JSON parser you are using. The ones I know should be able to deal with that. This would feel completely wrong if just for this sake I would give the second logjam check a name which doesn't contain the id of the first check.

Instead of using the value OK as severity when the host:port is not vulnerable instead use NOT VULNERABLE to add more clarity

OK is one of the eight severity levels and won't change.

For the finding field here too adding more consistency, see the following screenshot, for lucky13 the finding does not start with lucky13 whereas for rc4 it starts with RC4: not.

That was anyway on my list and has been addressed, see #970 .

and maybe more verbose in the explanation, something that could directly be used as a remediation.

verbosity in explanation should be limited by it's purpose. Again: JSON is a machine parseable out out. I still believe humans should be able to read it but not more. Therefore you need to have a look at the HTML or LOG output.

Remediation is another issue here. There were once suggestions but the project is difficult and is stale.

For the protocols section the ids for the protocols should be more consistent as well like we have got sslv2, sslv3 but then tls1, tls1_1 why not replacing them by tlsv1.0, tlsv1.1, etc.

It should be more consistent now (see branch referred by #970). I settled on the version with caps (acronym) and with underscore, the latter is subject to one night sleep over it.

Adding an additional field to the protocols section, it could be enabled: YES and enabled: NO or offered: YES and offered: NO so we could programmatically just test the value of this JSON field to determine if a protocol is being offered rather than working out if the finding field contains the string not offered.

That would require another field in fileout() just for this. Why not go with "offered" / "not offered"?
why no

[bao7uo]

I think you covered everything important in #970

[aress31]

@drwetter thanks for your quick reply.

Glad to see you have addressed some point. Regarding this point:

I am thinking about it for the not-so-close-future but I am not sure I like the blanks or too long ids. Last but not least it's a machine readable format. I still believe though humans should be able to read it but probably that goes too far.

Why not adding an additional field, such as displayName and it could be like BEAST, LOGJAM, etc. so you could keep the ids as they are for convenience.

Anyway thanks for your time.

Regards,
Alex

[drwetter]

Thanks for the feedback, Alex. Will consider it and changed the title accordingly.