Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

8262880: Add support for the NSS Key Log Format for SSL/TLS keys #2896

Closed
wants to merge 1 commit into from
Closed

8262880: Add support for the NSS Key Log Format for SSL/TLS keys #2896

wants to merge 1 commit into from

Conversation

SalusaSecondus
Copy link
Contributor

@SalusaSecondus SalusaSecondus commented Mar 9, 2021

This is my implementation for JDK-8262880 and enables creating of an SSL/TLS key log in the standardized NSS Key Log Format. This is supported by many TLS implementations and also by several parsers such as Wireshark. Supporting this will greatly ease in debugging TLS problems.

(Note: I am covered by the Amazon corporate contribution agreement).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issue

  • JDK-8262880: Add support for the NSS Key Log Format for SSL/TLS keys

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2896/head:pull/2896
$ git checkout pull/2896

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Mar 9, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 9, 2021

Hi @SalusaSecondus, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user SalusaSecondus" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@SalusaSecondus
Copy link
Contributor Author

/covered

@bridgekeeper bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Mar 9, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 9, 2021

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@openjdk
Copy link

openjdk bot commented Mar 9, 2021

@SalusaSecondus The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security security-dev@openjdk.org label Mar 9, 2021
@simonis
Copy link
Member

simonis commented Mar 9, 2021

I can confirm that @SalusaSecondus is covered by the Amazon OCA.

@bridgekeeper bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Mar 11, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 11, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 11, 2021

Webrevs

Copy link
Member

@XueleiFan XueleiFan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not good practice to leave secret information in debug log. Also, it may be not a good practice to introduce new logger format, including file and NSS format, into the SSLLogger. Someone also may want to introduce log format for MSS or XSS as well. Instead, please consider to make use of the features of Java Logger if you want to write the log to files, or use any special format.

@XueleiFan
Copy link
Member

/csr needed

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Mar 11, 2021
@openjdk
Copy link

openjdk bot commented Mar 11, 2021

@XueleiFan has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@SalusaSecondus please create a CSR request and add link to it in JDK-8262880. This pull request cannot be integrated until the CSR request is approved.

@simonis
Copy link
Member

simonis commented Mar 11, 2021

I'm happy to create a CSR for this issue once the exact details of the option format have been figured out.

@SalusaSecondus
Copy link
Contributor Author

SalusaSecondus commented Mar 11, 2021

I am not familiar with either the MSS or XSS log formats and would be interested to see them. The NSS format is a defacto industry standard and already supported by many libraries (both producers and consumers) and thus used widely in the security industry. Most other uses that I can find take this similar pattern of providing a file-name to the TLS logic and then getting key log (in this format) written to that file.

I agree completely that logging secret information is dangerous and should almost never be done. That is why it has to be explicitly enabled (unlike most of the other javax.net.debug options) and another reason it is not commingled with the other logging output.

@openjdk
Copy link

openjdk bot commented Mar 17, 2021

@SalusaSecondus Unknown command covered - for a list of valid commands use /help.

@SalusaSecondus
Copy link
Contributor Author

@XueleiFan I'd really like to move this forward but I'm uncertain what changes you want me to make. This extra debugging information will be very valuable to those of us debugging Java TLS connections.

@seanjmullan
Copy link
Member

I am also not comfortable adding this feature to the JDK, especially since every build of the JDK would by default have this feature enabled. Logging sensitive information to log files is not good security practice (there are many references I could cite). I also think it would be too easy to accidentally leave the system property enabled or forget to remove the file.

@SalusaSecondus
Copy link
Contributor Author

I think that there might be some confusion around the sensitivity of the data being logged. The security impact of this data is almost identical to passing -Djavax.net.debug=plaintext,packet to the JVM. This existing setting logs all plaintext (as well as wire-data) to STDERR. The new feature I'm proposing just lets a second application decrypt the wire-data to access the plaintext, which results in the same level of exposed data. (As an improvement over the existing feature, it outputs the data in a standardized format so that it is easier to analyze rather than being in a Java-specific format and co-mingled with all other STDERR output.)

Similar to the existing feature, the data logged only impacts the exact connections that it is enabled for. (These are ephemeral session-specific secrets).

I hope that this helps to ease some concerns and help explain why this exact feature is present in so many existing applications (including OpenSSL, BoringSSL, WolfSSL, s2n, Mozilla, and Chrome, among others). If you would like to try it with your current Chrome browser, just add --ssl-key-log-file=/path/to/chrome_keys.txt to the command-line. For Firefox, I believe you need to set the SSLKEYLOGFILE environment variable to the name of the log-file.

@mlbridge
Copy link

mlbridge bot commented Mar 23, 2021

Mailing list message from raell at web.de on security-dev:

Hi all,

I very much appreciate @SalusaSecondus' idea for providing a key logger.

For, when working with networks, it's quite natural to analyze message flow
with help of wireshark. In TLS 1.3 this isn't possible without a key log
because most parts of the handshake messages are encrypted. So, if both,
client and server, are Java apps, it isn't possible to trace the message flow
with wireshark. That's a great disadvantage.

To give an example: I'm running a https client based on SSLEngine and an
AsynchronousSocketChannel. The entries in the log provided by javax.net.debug=ssl
arise from SSLEngine, but the messages itself are sent by the channel. So it is
not possible to check what messages are actually sent over the network
(I could have made a programming error in using the data provided by
the SSLEngine or something could go wrong within the channel).
Therefore, being able to produce a key log on the Java client side and
to observe the messages and their content in wireshark, would be very
helpful.

Concerning the security issue: Of course, holding keys is always a risk.
But if one stores the output of javax.net.debug in a file, then an attacker
who has access to the system could read decrypted messages as well. But, of
course, the implementation of a key logger in Java has to ensure that no
key log is produced by default but only if it is explicitly enabled
(similar to javax.net.debug).

I would be very glad if there would be a chance to implement @SalusaSecondus'
proposal in some way because a key logger would be really helpful for
doing networking with Java.

Regards,

Ralph

?
?
?

Gesendet:?Freitag, 19. M?rz 2021 um 13:48 Uhr
Von:?"Sean Mullan" <mullan at openjdk.java.net>
An:?security-dev at openjdk.java.net
Betreff:?Re: RFR: 8262880: Add support for the NSS Key Log Format for SSL/TLS keys
On Thu, 18 Mar 2021 21:26:28 GMT, SalusaSecondus <github.com+829871+SalusaSecondus at openjdk.org> wrote:

It is not good practice to leave secret information in debug log. Also, it may be not a good practice to introduce new logger format, including file and NSS format, into the SSLLogger. Someone also may want to introduce log format for MSS or XSS as well. Instead, please consider to make use of the features of Java Logger if you want to write the log to files, or use any special format.

@XueleiFan I'd really like to move this forward but I'm uncertain what changes you want me to make. This extra debugging information will be very valuable to those of us debugging Java TLS connections.

I am also not comfortable adding this feature to the JDK, especially since every build of the JDK would by default have this feature enabled. Logging sensitive information to log files is not good security practice (there are many references I could cite). I also think it would be too easy to accidentally leave the system property enabled or forget to remove the file.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2896

@mlbridge
Copy link

mlbridge bot commented Mar 23, 2021

Mailing list message from Bernd Eckenfels on security-dev:

Hello,

I agree with the need for such a facility.

Instead, or in addition to a key log a session handshake listener with access to the handshake result (and the master key) would also be useful for auditing, renegotiation limiting and key logging. It can even be used for things like priming introspecting firewalls.

There is not yet a good official platform API with access to that kind of information (and proxying client Hello packages to get the offered ciphers is really ugly). With the API an Adapter which can write NSS Keylogs can be provided by users and you don?t have to worry about having debug logging Code expose secrets.

Gruss
Bernd

--
http://bernd.eckenfels.net
________________________________
Von: security-dev <security-dev-retn at openjdk.java.net> im Auftrag von raell at web.de <raell at web.de>
Gesendet: Tuesday, March 23, 2021 6:36:06 PM
An: security-dev at openjdk.java.net <security-dev at openjdk.java.net>
Betreff: Re: RFR: 8262880: Add support for the NSS Key Log Format for SSL/TLS keys

Hi all,

I very much appreciate @SalusaSecondus' idea for providing a key logger.

For, when working with networks, it's quite natural to analyze message flow
with help of wireshark. In TLS 1.3 this isn't possible without a key log
because most parts of the handshake messages are encrypted. So, if both,
client and server, are Java apps, it isn't possible to trace the message flow
with wireshark. That's a great disadvantage.

To give an example: I'm running a https client based on SSLEngine and an
AsynchronousSocketChannel. The entries in the log provided by javax.net.debug=ssl
arise from SSLEngine, but the messages itself are sent by the channel. So it is
not possible to check what messages are actually sent over the network
(I could have made a programming error in using the data provided by
the SSLEngine or something could go wrong within the channel).
Therefore, being able to produce a key log on the Java client side and
to observe the messages and their content in wireshark, would be very
helpful.

Concerning the security issue: Of course, holding keys is always a risk.
But if one stores the output of javax.net.debug in a file, then an attacker
who has access to the system could read decrypted messages as well. But, of
course, the implementation of a key logger in Java has to ensure that no
key log is produced by default but only if it is explicitly enabled
(similar to javax.net.debug).

I would be very glad if there would be a chance to implement @SalusaSecondus'
proposal in some way because a key logger would be really helpful for
doing networking with Java.

Regards,

Ralph

Gesendet: Freitag, 19. M?rz 2021 um 13:48 Uhr
Von: "Sean Mullan" <mullan at openjdk.java.net>
An: security-dev at openjdk.java.net
Betreff: Re: RFR: 8262880: Add support for the NSS Key Log Format for SSL/TLS keys
On Thu, 18 Mar 2021 21:26:28 GMT, SalusaSecondus <github.com+829871+SalusaSecondus at openjdk.org> wrote:

It is not good practice to leave secret information in debug log. Also, it may be not a good practice to introduce new logger format, including file and NSS format, into the SSLLogger. Someone also may want to introduce log format for MSS or XSS as well. Instead, please consider to make use of the features of Java Logger if you want to write the log to files, or use any special format.

@XueleiFan I'd really like to move this forward but I'm uncertain what changes you want me to make. This extra debugging information will be very valuable to those of us debugging Java TLS connections.

I am also not comfortable adding this feature to the JDK, especially since every build of the JDK would by default have this feature enabled. Logging sensitive information to log files is not good security practice (there are many references I could cite). I also think it would be too easy to accidentally leave the system property enabled or forget to remove the file.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2896
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210323/56c0b9e6/attachment-0001.htm>

@jnimeh
Copy link
Member

jnimeh commented Mar 24, 2021

I agree with Bernd, an API gives us some more flexibility. I like the functionality provided by a key logging feature, but an API-based approach is more appropriate for the platform, and it more closely resembles what OpenSSL and GnuTLS are doing with callback registration.

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 21, 2021

@SalusaSecondus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented May 19, 2021

@SalusaSecondus This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csr Pull request needs approved CSR before integration rfr Pull request is ready for review security security-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

5 participants