Generator-jhipster: Exception Handling: Leakage of Implementation Details

[AlexandreCassagne]

Overview of the issue

Some exceptions trigger a leakage of implementation details to the end user. The example shown here relates to Jackson's exception details, but maybe there are other examples that I am not aware of.

Motivation for or Use Case

We recently underwent security testing -- one of the issues that was caught is that certain exceptions leak inside information. An example is shown below.

Reproduce the error

Use a malformed JSON request body.

Example Request

GET /auth/login

35 {"username":"asd","password":"asd","rememberMe":true} 1
Z
Q

Example Response

{
    "type": "https://www.jhipster.tech/problem/problem-with-message",
    "title": "Bad Request",
    "status": 400,
    "detail": "JSON parse error: Cannot deserialize instance of 
       `java.util.LinkedHashMap<java.lang.Object,java.lang.Object>` out of VALUE_NUMBER_INT token; nested exception is com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot deserialize instance of `java.util.LinkedHashMap<java.lang.Object,java.lang.Object>` 
       out of VALUE_NUMBER_INT token\n at [Source: (PushbackInputStream); line: 1, column: 1]", 
    "path": "/auth/login",
    "message": "error.http.400"
}

As can be seen above, the error message discloses internal details that could help an attacker understand the system's vulnerabilities better.

In this case, "The input is not valid JSON: expecting a JSON object but found a number instead." is a better message because it better communicates the error without disclosing any implementation details. However, if it is too complicated to implement such detailed error messages, we should simply say that the JSON is malformed (this is what I have done, as I am not familiar enough with Jackson to go any further).

Related issues

Suggest a Fix

It is easy to fix this particular exception (I have done so on my end, using if statements in ExceptionTranslator#process(...)).

However, there may be other examples that I am not aware. If you have any examples in mind, please comment below or create a new issue!

JHipster Version(s)

Reproduced on both 6.7.1 Gateway and 6.9.1 micro-service. I haven't seen a fix in recent versions' change logs; correct me if I am wrong.

JHipster configuration

.yo-rc

{

  "generator-jhipster": {

    "promptValues": {

      "packageName": "com.*.gateway",

      "nativeLanguage": "en"

    },

    "jhipsterVersion": "6.7.1",

    "applicationType": "gateway",

    "baseName": "gateway",

    "packageName": "com.nxtsens.backend.gateway",

    "packageFolder": "com/nxtsens/backend/gateway",

    "serverPort": "8080",

    "authenticationType": "uaa",

    "uaaBaseName": "uaa",

    "cacheProvider": "hazelcast",

    "enableHibernateCache": true,

    "websocket": "spring-websocket",

    "databaseType": "sql",

    "devDatabaseType": "mysql",

    "prodDatabaseType": "mysql",

    "searchEngine": false,

    "messageBroker": "kafka",

    "serviceDiscoveryType": "eureka",

    "buildTool": "gradle",

    "enableSwaggerCodegen": false,

    "clientFramework": "react",

    "useSass": true,

    "clientPackageManager": "yarn",

    "testFrameworks": ["cucumber"],

    "jhiPrefix": "jhi",

    "enableTranslation": true,

    "nativeLanguage": "en",

    "languages": ["en", "fr"],

    "entitySuffix": "",

    "dtoSuffix": "DTO",

    "otherModules": [],

    "clientTheme": "none",

    "blueprints": [],

    "embeddableLaunchScript": false,

    "creationTimestamp": 1579029215188

  }

}

  

• [x] Checking this box is mandatory (this is just to show you read everything)

[atomfrede]

This is indeed not good. The problem MessageNotReadableAdviceTrait uses the default prepare method which uses the message of the exception as details.

We could overwrite the prepare method in ExceptionTranslator and e.g. check for certain exceptions or try to prevent the leaking details by checking for certain keywords, e.g.

    public ProblemBuilder prepare(final Throwable throwable, final StatusType status, final URI type) {
        if (throwable instanceof HttpMessageConversionException) {
            return Problem.builder()
                .withType(type)
                .withTitle(status.getReasonPhrase())
                .withStatus(status)
                .withDetail("Invalid http message") // <-- This should be "clear" but generic enough to not leak details
                .withCause(Optional.ofNullable(throwable.getCause())
                    .filter(cause -> isCausalChainsEnabled())
                    .map(this::toProblem)
                    .orElse(null));
        }
       // This is the default from the AdviceTrait interface
        return Problem.builder()
            .withType(type)
            .withTitle(status.getReasonPhrase())
            .withStatus(status)
            .withDetail(throwable.getMessage())  // <-- This leaks the details
            .withCause(Optional.ofNullable(throwable.getCause())
                .filter(cause -> isCausalChainsEnabled())
                .map(this::toProblem)
                .orElse(null));
    }

I am not sure if we can provide custom translations (like for constraint validation exceptions) for such runtime exceptions.

[atomfrede]

I found another exception (org.hibernate.exception.DataException) which may leak details to the caller:

{
  "type" : "https://www.jhipster.tech/problem/problem-with-message",
  "title" : "Internal Server Error",
  "status" : 500,
  "detail" : "could not execute batch; SQL [update label set label=? where id=?]; nested exception is org.hibernate.exception.DataException: could not execute batch",
  "path" : "/api/labels",
  "message" : "error.http.500"
}

[atomfrede]

Possible solution which handles certain exception to write more detailed message while preventing messages that contain potential technical details (in this case quite simple checking for package names) to the caller:

```java
public ProblemBuilder prepare(final Throwable throwable, final StatusType status, final URI type) {
if (throwable instanceof HttpMessageConversionException) {
return Problem.builder()
.withType(type)
.withTitle(status.getReasonPhrase())
.withStatus(status)
.withDetail("Unable to convert http message")
.withCause(Optional.ofNullable(throwable.getCause())
.filter(cause -> isCausalChainsEnabled())
.map(this::toProblem)
.orElse(null));
}

    if (throwable instanceof DataAccessException) {
        return Problem.builder()
            .withType(type)
            .withTitle(status.getReasonPhrase())
            .withStatus(status)
            .withDetail("Failure during data access")
            .withCause(Optional.ofNullable(throwable.getCause())
                .filter(cause -> isCausalChainsEnabled())
                .map(this::toProblem)
                .orElse(null));
    }

    if (containsPackageName(throwable.getMessage())) {
        return Problem.builder()
            .withType(type)
            .withTitle(status.getReasonPhrase())
            .withStatus(status)
            .withDetail("Unexpected runtime exception")
            .withCause(Optional.ofNullable(throwable.getCause())
                .filter(cause -> isCausalChainsEnabled())
                .map(this::toProblem)
                .orElse(null));
    }

    return Problem.builder()
        .withType(type)
        .withTitle(status.getReasonPhrase())
        .withStatus(status)
        .withDetail(throwable.getMessage())
        .withCause(Optional.ofNullable(throwable.getCause())
            .filter(cause -> isCausalChainsEnabled())
            .map(this::toProblem)
            .orElse(null));
}

private boolean containsPackageName(String message) {

    // This list is for sure not complete
    return StringUtils.containsAny(message, "org.", "java.", "net.", "javax.", "com.", "io.", "de.");
}

[atomfrede]

@jhipster/developers If we all agree on the above solution I can provide a PR as having potential (internal) technical details by accident revealed via the api is not good.

[cbornet]

If we do it, it should be limited to the prod profile.

[atomfrede]

@cbornet Yes, good point, makes sense.

[gmarziou]

@atomfrede does your implementation impact also logging or only http response?

Also, I think this should be documented in https://www.jhipster.tech/production/#-security

[atomfrede]

The log still shows the detailed error message even when the response just states "Unable to convert http message". And yes we should document this.

[AlexandreCassagne]

The log still shows the detailed error message even when the response just states "Unable to convert http message".

Ideally, we should implement specific error messages for a wide variety of exception types. One of the issue is that the quality of certain exceptions is low (looking at database messages for instance, Postgres implements high quality metadata in its exceptions, but MySQL doesn't make it easy to interpret exceptions).

[atomfrede]

If you have some specific ones already in mind feel free to provide them. I will do two things

• Provide a PR similar to the outlined approach (6.x and 7.x)
• Document it and explicitly state if you need more specific messaged you have to handle them