swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
17.02k stars 6.03k forks source link

Coverity giving High Impact Security Issue in API Client code for applySslSettings() method #9911

Open ankushso opened 4 years ago

ankushso commented 4 years ago

Coverity is raising security issues for TrustManager in highlighted code below

Insecure SSL/TLS: bad TrustManager High impact security generatedChild/src/main/java/com/amdocs/oss/sfo/serviceordermanager/child/ApiClient.java ApiClient$2.checkServerTrusted
Insecure SSL/TLS: bad HostnameVerifier High impact security generated/src/main/java/io/swagger/client/ApiClient.java ApiClient$3.verify
Insecure SSL/TLS: bad TrustManager High impact security generated/src/main/java/io/swagger/client/ApiClient.java ApiClient$2.checkClientTrusted

TrustManager[] trustManagers = null; HostnameVerifier hostnameVerifier = null; if (!verifyingSsl) { TrustManager trustAll = new X509TrustManager() { @Override public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {} @Override public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException {} @Override public X509Certificate[] getAcceptedIssuers() { return null; } }; SSLContext sslContext = SSLContext.getInstance("TLS"); trustManagers = new TrustManager[]{ trustAll }; hostnameVerifier = new HostnameVerifier() { @Override public boolean verify(String hostname, SSLSession session) { return true; } };

GowthamJaganathhan commented 4 years ago

Hi @ankushso ,

Was this sorted, any leads to the solution?

oriassaraf commented 3 years ago

SonarQuob gives similar error (Blocker Security Vulnerability)- "HostnameVerifier.verify" should not always return true To prevent URL spoofing, HostnameVerifier.verify() methods should do more than simply return true. Doing so may get you quickly past an exception, but that comes at the cost of opening a security hole in your application.

we are using codegen version 2.3.1 - was this fixed in later version?

frantuma commented 3 years ago

We are considering it not to represent a vulnerability, as the affected code branch would only be executed if explicitly configured by the user, as the default behaviour is to apply SSL verification.

To elaborate a bit more, a user can run swagger-codegen with appropriate options to generate a java client using okhttp-gson library from a Swagger/OpenAPI specification; such code includes the code branch based on the value of verifyingSsl variable (the branch is executed if the variable is false), which is always set to true by code generation, there is no option at all to generate code with such variable set to false.

The user can then, in his own code, make use of the generated client library; it is at this point that he might be able to explicitly configure the generated client to skip SSL validation, by instantiating the client and setting its verifyingSsl to false. This is however equivalent to deliberately run some insecure code, e.g. by configuring SSL for httpClient as he wishes, regardless of any provided code, or by performing any other insecure action.

We therefore consider this to be a false positive of analysis tools, and not a real vulnerability on its own.

ilatypov commented 1 year ago

The older "okhttp" generator does not have a generator-time option for a source-code enforced version.

If the server is authenticated, its certificate message must provide a valid certificate chain leading to an acceptable certificate authority. https://www.rfc-editor.org/rfc/rfc5246#appendix-F.1.1

Instead of a blunt bypass of F.1.1, the generator could have run-time methods

In addition to the weaknesses pointed here, Fortify points to a "weak SSL protocol" allowed by the following snippet (before the Java 8, 11 updates issued few days ago),

SSLContext sslContext = SSLContext.getInstance("TLS");

The older "okhttp" generator appears to micro-manage the TLS connection parameters as a (probably unnecessary) side effect of okhttp having to avoid some shared usage conflicts related to ALPN. https://github.com/square/okhttp/blob/okhttp_27/okhttp/src/main/java/com/squareup/okhttp/OkHttpClient.java#L617-L639

The newer "okhttp4" generator relies on the newer "okhttp3" package that seems to have done away with the earlier work-around and to rely on regular run-time properties. https://github.com/square/okhttp/blob/parent-4.11.0/okhttp/src/main/kotlin/okhttp3/OkHttpClient.kt#L177

So perhaps the okhttp4 generator enforces the safe client functionality and allows some adjustments using runtime system properties for crypto and HTTP. Developers then should treat the older okhttp generator with the same contempt as older unsafe binary component releases.

The generators moved to a separate project but the same upgrade path from an okhttp generator to the okhttp4 one exists there. https://github.com/swagger-api/swagger-codegen-generators/tree/v1.0.42/src/main/resources/handlebars/Java/libraries