search

jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.54k stars 4.02k forks source link

Bad sonar security rating #14949

Closed atomfrede closed 2 years ago

atomfrede commented 3 years ago
Overview of the issue

Because of a change in the sonar rulesets we have now just a B in terms of security in our sample application. The issue is imho not severe (maybe we can also exclude/ignore it), but we should do something about the red quality gate.

image

image

atomfrede commented 3 years ago

I tried to forge some logs and succeeded. Running locally you can easily see the entry has been forged as it is not colored, but inside a log file or some other tool that would not be possible. So I tried OWASP easpi to encode it, but didn't have something working here.

atomfrede commented 3 years ago

I have managed to configured https://github.com/javabeanz/owasp-security-logging with the owasp filter documented here https://github.com/javabeanz/owasp-security-logging/wiki/Log-Forging

It looks like this. Maybe we should only enabled it in production as the nice "table like" log gets broken of course. You can see how the line break is replaced by the owasp clrf filter:

image

Basically this requires defining the clrf filter in logback-spring.xml and setting logging.pattern.console via regular properties using the clrf filter. With this I would say we are doing fine against log forging and could even ignore the sonar warnings regarding log forging. WDYT?

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Our core developers tend to be more verbose on denying. If there is no negative comment, possibly this feature will be accepted. We are accepting PRs :smiley:. Comment or this will be closed in 7 days

atomfrede commented 3 years ago

Keep it open

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Our core developers tend to be more verbose on denying. If there is no negative comment, possibly this feature will be accepted. We are accepting PRs :smiley:. Comment or this will be closed in 7 days

atomfrede commented 3 years ago

Keep it open

nomuna commented 3 years ago

Just cloned the source code for the project and ran a sonar analysis with sonarqube-9.0.1.46107 for windows x64 with the defaults/ sonar quality gates. I don't see any security issues... Am I missing something? Is the project on sonar cloud? If it is could you provide the url?

atomfrede commented 3 years ago

It's the generated sample app not the generator code itself. It is on sonar cloud here https://sonarcloud.io/dashboard?id=jhipster-sample-application

nomuna commented 3 years ago

It's the generated sample app not the generator code itself. It is on sonar cloud here https://sonarcloud.io/dashboard?id=jhipster-sample-application

So it has nothing to do with the code under https://github.com/jhipster/jhipster-sample-app ?

atomfrede commented 3 years ago

It is exactly that project, you are right.

atomfrede commented 3 years ago

But in fact the rating has changed from F to B compared to the screenshots. The log forging is now minor as it seems

nomuna commented 3 years ago

Screenshot 2021-09-07 083703

But in fact the rating has changed from F to B compared to the screenshots. The log forging is now minor as it seems

It is seriously weird... The scan I performed is a couple of hrs old and the code was checked out may be at the same time. I don't see those log forging vulnerabilities. Was there a version change and may be they changed their quality gates? Is it possible?

nomuna commented 3 years ago

BankAccountQueryService class is neither in master nor in main branch. I think the code comes from a different repository. Or very old and the class in question was deleted long time ago...

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Our core developers tend to be more verbose on denying. If there is no negative comment, possibly this feature will be accepted. We are accepting PRs :smiley:. Comment or this will be closed in 7 days

atomfrede commented 3 years ago

Keep it open

RobertBurrellDonkin commented 2 years ago

I think that https://sonarcloud.io/dashboard?id=jhipster-sample-application is failing now just on code coverage.

What am I missing...?

nomuna commented 2 years ago

I think that https://sonarcloud.io/dashboard?id=jhipster-sample-application is failing now just on code coverage.

What am I missing...?

As I have pointed out in my last message the project has moved on, some classes were deleted and refactored so that the screenshot used as info for the issue is no longer relevant. The devs refuse to accept it apparently... :D IMHO the issue should be closed.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Our core developers tend to be more verbose on denying. If there is no negative comment, possibly this feature will be accepted. We are accepting PRs :smiley:. Comment or this will be closed in 7 days

pascalgrimaud commented 2 years ago

Probably it's because the Sonar analysis is broken since June:

image

atomfrede commented 2 years ago

Coming back to this. The issue still exists, but the vulnerabilities are now minor (B). Nevertheless log forging is still possible, while I suppose a lot of people are using json based/structured logging instead of plain pattern layouts, not sure we must do something about it. We could document it more clearly in "going to production" and ignore the sonar issues. In addition we may also provide json log configuration (by default) for production?