Add SECURITY.md

[JamieSlome]

Hey there!

I belong to an open source security research community, and a member (@michaellrowley) has found an issue, but doesn’t know the best way to disclose it.

If not a hassle, might you kindly add a SECURITY.md file with an email, or another contact method? GitHub recommends this best practice to ensure security issues are responsibly disclosed, and it would serve as a simple instruction for security researchers in the future.

Thank you for your consideration, and I look forward to hearing from you!

(cc @huntr-helper)

[solardiz]

Thank you for the reminder. I think at this time a better name would have been INSECURITY, as I had suggested here:

https://github.com/openwall/john/issues/3513#issuecomment-447601798

I think we should very explicitly state in the documentation, in some place like a text file called INSECURITY, that JtR isn't to be used on untrusted input and should be run with the lowest privileges necessary.

JtR jumbo being unsuitable for handling of untrusted input is a consequence of how many formats and different parsers we have. It is unrealistic to have zero bugs there. We should document that.

Separately, we should also work, resources permitting, on making the code more robust - such as through more consistent implementations of valid that would invoke shared sanity-checkers. We already have those for numbers embedded in hash encodings. We could also have e.g. regexp-alikes covering the whole strings, where a match is mandatory before we do any further parsing.`

and @magnumripper agreed:

I agree about that INSECURITY file. It can be completely general about the whole package, no real need to single out any specific format. This is all best-effort (or worse).

MPI could be singled out, although the problem is mainly with MPI itself and not with JtR. I already added warnings to our docs.

However, since SECURITY.md is a standard file name, I think we should in fact create that, but explain inside it that for security purposes all input is considered trusted, and it should be assumed that input can control the program in arbitrary ways, even though for robustness (not yet security) we're gradually removing such possibilities (such as with @AlekseyCherepanov's current work on addressing the findings of our recent fuzzing). We can also suggest that in cases where greater robustness is desired, even if at occasional usability expense, the --format option can be used to reduce the parsing of input files.

We also already do mention in doc/EXTERNAL that external mode programs in john.conf can control the program, so (at least) that establishes john.conf and the files it includes and a possible replacement specified with --config as trusted input.

The mentioned problem with MPI is that in --enable-mpi builds (not default), an MPI library tends to open a listening port, and can expose itself and john and the underlying system to direct network-based attacks as well.

As to instructions on where to report, given the status and stance above, please just report issues in public right away.

Edit: added more quotes from the referenced older discussion, described the MPI problem.

[solardiz]

@michaellrowley Please read the comment above, and then create a public issue in here for what you found. Knowing that might also help us make sure the SECURITY.md we'd create would have addressed your question. Thank you! (cc @huntr-helper)

[michaellrowley]

Thank you for your detailed response - the vulnerability that I reported was a null pointer dereference which would only have occured in low-resource situations and would probably have resulted in a crash so I'm not sure that it warrants an issue (if you'd still like me to open one, I can do that.)

With regards to the SECURITY.md file, some of the below links might be useful: https://www.ntia.doc.gov/files/ntia/publications/ntia_vuln_disclosure_early_stage_template.pdf https://securitytxt.org/ https://github.com/cncf/tag-security/blob/main/project-resources/templates/SECURITY.md (SECURITY.md template) https://github.com/dec0dOS/amazing-github-template https://opensource.ieee.org/community/manual/-/wikis/SECURITY.md (Example SECURITY.md from IEEE)

For security purposes all input is considered trusted, and it should be assumed that input can control the program in arbitrary ways.

This would disqualify most vulnerabilities so I'd urge you to consider making that a clear point in the README.md (or somewhere else that users are likely to see it) to make sure that there is a cohesive understanding of the project's security status from researchers to users.

(I can see the rationale behind having an INSECURITY.md filename but I'm pretty sure that people/automated bots will be expecting SECURITY.md due to it being a GitHub recommendation.)

[solardiz]

null pointer dereference which would only have occured in low-resource situations

Sounds like a missed NULL check after malloc() or such, so a bug to fix. Ideally, you'd send us a PR fixing it right away, but if you can't fix it yet feel free to open an issue. We wouldn't treat that as a vulnerability, but we'd like to make the code more robust as our resources permit. Thanks.

[JamieSlome]

Just for reference, the report can be found here:

https://huntr.dev/bounties/315d2d54-6ae4-47a1-a6ff-84fea6cb7d01/

It is private and only accessible to maintainers with repository push permissions! ❤️

[solardiz]

@JamieSlome Thank you, it's an interesting platform you run, but of course no reason to treat this issue reported via it as non-public. May I just create a public issue in here and copy-paste the info from there?

[JamieSlome]

@solardiz - seeing as you are one of the maintainers, it is up to you ❤️

[solardiz]

@JamieSlome @huntr-helper Incidentally, the huntr platform doesn't appear to let a maintainer approve the bug report, but decline security relevance. There is a way to specify that user interaction is required, which lowers severity, but there's no way to specify that the input is considered trusted, which would make the issue non-security. Something you might want to add.