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

Cryptographic Issue #9032

Closed
sureshkrishnamoorthy opened this issue Apr 11, 2019 · 5 comments · Fixed by #9041
Closed

Cryptographic Issue #9032

sureshkrishnamoorthy opened this issue Apr 11, 2019 · 5 comments · Fixed by #9041
Milestone

Comments

@sureshkrishnamoorthy
Copy link

sureshkrishnamoorthy commented Apr 11, 2019

Software quality and security analysis tool (Veracode) reports an error "Improper Validation of Certificate with Host Mismatch (CWE ID 297)(1 flaw)" in
https://github.com/netty/netty/blob/netty-4.1.34.Final/handler/src/main/java/io/netty/handler/ssl/OpenSslX509TrustManagerWrapper.java
Description : The failure to validate host-specific certificate data may mean that, while the certificate read was valid, it was not for the site originally requested.

Netty version : 4.1.34.Final

Please let us know if it is a valid issue, if
yes, please suggest how to mitigate this.
no, please provide justification

@normanmaurer
Copy link
Member

Sorry but you will have to provide more details.

@sureshkrishnamoorthy
Copy link
Author

Veracode vulnerability analysis reports vulnerability at line no 69 in OpenSslX509TrustManagerWrapper.java ie
`

                        context.init(null, new TrustManager[] {
                                                new X509TrustManager() {
                        @Override
                        public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
                                throws CertificateException {
                        }
                        @Override
                        public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
                                throws CertificateException {
                        }

                        @Override
                        public X509Certificate[] getAcceptedIssuers() {
                            return EmptyArrays.EMPTY_X509_CERTIFICATES;
                        }
                    }
            }, null);

`
Reported Issue: Improper Validation of Certificate with Host Mismatch (CWE ID 297)

We assume that in the above overridden methods doesn't have any implementation which might be causing the vulnerability issue.
Please let us know of your views on this issue.

@normanmaurer
Copy link
Member

@sureshkrishnamoorthy this is not a "valid report" as this is only used to test if a wrapper is applied and is never used to actual verify certificates at the end.

That said I can make this go away easily. Let me do this to make things "easier" for you.

normanmaurer added a commit that referenced this issue Apr 12, 2019
…ccept

Motivation:

Seems like some analyzer / validation tools scan code to detect if it may produce some security risk because of just blindly accept certificates. Such a tool did tag our code because we have such an implementation (which then is actually never be used). We should just change the impl to not do this as it does not matter for us and it makes such tools happier.

Modifications:

Throw CertificateException

Result:

Fixes #9032
@normanmaurer normanmaurer added this to the 4.1.35.Final milestone Apr 12, 2019
@normanmaurer
Copy link
Member

@sureshkrishnamoorthy fixed by #9041

normanmaurer added a commit that referenced this issue Apr 12, 2019
…ccept (#9041)

Motivation:

Seems like some analyzer / validation tools scan code to detect if it may produce some security risk because of just blindly accept certificates. Such a tool did tag our code because we have such an implementation (which then is actually never be used). We should just change the impl to not do this as it does not matter for us and it makes such tools happier.

Modifications:

Throw CertificateException

Result:

Fixes #9032
normanmaurer added a commit that referenced this issue Apr 12, 2019
…ccept (#9041)

Motivation:

Seems like some analyzer / validation tools scan code to detect if it may produce some security risk because of just blindly accept certificates. Such a tool did tag our code because we have such an implementation (which then is actually never be used). We should just change the impl to not do this as it does not matter for us and it makes such tools happier.

Modifications:

Throw CertificateException

Result:

Fixes #9032
@sureshkrishnamoorthy
Copy link
Author

@normanmaurer Thank you so much for quick response and action. Do we have the release date for 4.1.35.Final version ?

alfred-yeung added a commit to riptano/netty that referenced this issue May 28, 2022
…ccept (netty#9041)

Motivation:
Seems like some analyzer / validation tools scan code to detect if it may produce some
security risk because of just blindly accept certificates. Such a tool did tag our code
because we have such an implementation (which then is actually never be used). We should
just change the impl to not do this as it does not matter for us and it makes such tools
happier.

Modifications:
Throw CertificateException

Result:
Fixes (netty#9032)
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

Successfully merging a pull request may close this issue.

2 participants