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

SQL Injection in Reactive project #18269

Closed
appkr opened this issue Apr 3, 2022 · 35 comments · Fixed by #18294
Closed

SQL Injection in Reactive project #18269

appkr opened this issue Apr 3, 2022 · 35 comments · Fixed by #18294
Labels
area: bug 🐛 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ theme: reactive ⚛️ Spring WebFlux theme: security $500 https://www.jhipster.tech/bug-bounties/
Milestone

Comments

@appkr
Copy link

appkr commented Apr 3, 2022

I don't know whether this is the right place to ask this though...

In a reactive spring project with r2dbc, I found that SQL injection is actually possible. This may happen because of me, lacking knowledge on how to use r2dbc correctly. If it is case please let me know the correct usage.

Setup

  • Jhipster 7.7.0
  • reactive with Spring WebFlux? Yes
  • type of database? SQL

JDL

entity Example {
	id String
  name String
}

Change I made

  • Make /api/** be accessible without authorization header
  • Make GET /api/examples accept query parameter of name, and bind the parameter to the findAllBy(Pageable pageable, Criteria criteria) repository method
  • Make a request to GET /api/examples?name=foobar';DROP TABLE example;--

Result

  • The sql rendered was SELECT e.id AS e_id, e.name AS e_name FROM example e WHERE name = 'foobar';DROP TABLE example;--'
  • That actually dropped the table

스크린샷 2022-04-04 오전 7 53 32

@pascalgrimaud
Copy link
Member

Cc @jhipster/developers

@atomfrede
Copy link
Member

Didn't expect that. Is it the same for non reactive/hibernate?

@pascalgrimaud
Copy link
Member

I think it's important to try and check

@jdubois
Copy link
Member

jdubois commented Apr 4, 2022

On non-reactive, at least when I worked on it a few years ago, we were using Hibernate’s criteria API, which would prevent this.

@pascalgrimaud pascalgrimaud added area: bug 🐛 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $500 https://www.jhipster.tech/bug-bounties/ and removed area: needs-reproduction labels Apr 4, 2022
@deepu105
Copy link
Member

deepu105 commented Apr 4, 2022

For non reactive i'll be extremely surprised if its the case since we don't do any direct SQL and everything goes via Spring Data and hibernate

@atomfrede
Copy link
Member

Yes me too. Maybe thats also the reason why we didn't check it for reactive part in detail. Spoiled by hibernate's power 😄

@deepu105
Copy link
Member

deepu105 commented Apr 4, 2022

we should be using named parameters isn't it? seems like here its generating with value instead

@OmarHawk
Copy link
Contributor

OmarHawk commented Apr 4, 2022

Probably, the Conditions.just taking a literal sql here allowing an arbitrary WHERE clause, but also being vulnerable for injection:

return createSelectImpl(selectFrom.limitOffset(pageable.getPageSize(), pageable.getOffset()).where(Conditions.just(criteria.toString())), entityType, pageable.getSort());

and here:

return createSelectImpl(selectFrom.where(Conditions.just(criteria.toString())), entityType, null);

@OmarHawk
Copy link
Contributor

OmarHawk commented Apr 4, 2022

Probably came out of the following PR #13515

@deepu105
Copy link
Member

deepu105 commented Apr 4, 2022

@atomfrede are you working on this? else I can try to find a fix?

@atomfrede
Copy link
Member

Wanted to start now.

@deepu105
Copy link
Member

deepu105 commented Apr 4, 2022

I have a CVE drafted for this ready to go once we have a fix in place. Let me know if you need any help

@atomfrede
Copy link
Member

At least I can reproduce it too pretty easy :(

@atomfrede
Copy link
Member

atomfrede commented Apr 4, 2022

When mapping the criteria manually to a condition it is not vulnerable, but somehow this seems to me as not the intended way to use spring datas criteria. But using .just(VALUE) instead of .just(WHOLE CRITERIA) seems to do the trick. But creating the custom mapping will be a lot of code and most likely quite error prone.

@mraible Maybe you have come across this?

//Criteria.where("name").is("foobar';DROP TABLE example;--");
//-->toString -->'foobar';DROP TABLE example;--'
//name=foobar';DROP TABLE example;--
Comparison equal = Conditions.isEqual(
Column.create(criteria.getColumn(), Table.create("example")),
                    Conditions.just(criteria.getValue().toString()));

@atomfrede
Copy link
Member

I have created a reproducer here https://github.com/atomfrede/reactive-sql-jhipster-reproducer which has a potential hand coded fix. Try it via http://localhost:8080/api/examples?name=foobar';DROP TABLE example;--. Without the changes it will indeed delete the table while with the hand coded condition it works correctly.

@deepu105
Copy link
Member

deepu105 commented Apr 5, 2022

@atomfrede how important is the where clause mapping? can we remove the feature for the time being if it only affects searching in Get queries? so that we can publish a fix and then look for a proper solution?

@atomfrede
Copy link
Member

As far as I can tell the generated application does not need it by default I think. All crud operations should work without the custom criteria (need to check). So yes we might remove it for the time being.

@deepu105
Copy link
Member

deepu105 commented Apr 5, 2022

In that case, I vote to remove it and publish a patch and CVE. Then we can create a new issue to add the feature in a secure and scalable way (if it's important). @jdubois @pascalgrimaud @mraible @DanielFran @jhipster/developers WDYAT?

@pascalgrimaud
Copy link
Member

Agree with this fix. We can see later if we can do better

@atomfrede
Copy link
Member

So basically we will ignore the Criteria provided to EntityManager.createSelect right?

@deepu105
Copy link
Member

deepu105 commented Apr 5, 2022

yes, and maybe change signature in generated code, if possible, to not take the criteria and add a comment so that users don't end up adding the same manually

@pascalgrimaud pascalgrimaud added this to the 7.8.1 milestone Apr 5, 2022
@DanielFran
Copy link
Member

Yes, I agree too with this fix.

@deepu105
Copy link
Member

deepu105 commented Apr 7, 2022

Thanks to everyone involved @appkr @OmarHawk we have decided to award each of you a 300$ bounty so please claim it via open collective. @atomfrede for you, we (me, Julien, Pascal) think you deserve more than 500$ so I'll send a mail in the public group to discuss that.

@appkr
Copy link
Author

appkr commented Apr 7, 2022

@deepu105 Thanks, the claim was filed at https://opencollective.com/generator-jhipster/expenses/71638

@deepu105
Copy link
Member

deepu105 commented Apr 7, 2022

@appkr it's approved. Next time please do report security issues to project maintainers privately first so we can disclose it responsibly after fixing them.

@appkr
Copy link
Author

appkr commented Apr 7, 2022

@deepu105 My bad, sorry. Yes I will If I have any.

@OmarHawk
Copy link
Contributor

OmarHawk commented Apr 7, 2022

Thanks to everyone involved @appkr @OmarHawk we have decided to award each of you a 300$ bounty so please claim it via open collective. @atomfrede for you, we (me, Julien, Pascal) think you deserve more than 500$ so I'll send a mail in the public group to discuss that.

Thanks :-). Filed it at https://opencollective.com/generator-jhipster/expenses/71657

@mshima
Copy link
Member

mshima commented Apr 7, 2022

@deepu105 it's missing to tag the release at GitHub https://github.com/jhipster/generator-jhipster/releases.

@mraible
Copy link
Contributor

mraible commented Apr 7, 2022

I noticed there are two draft releases on the releases page. We should fix that and remove all the dependabot updates. As a developer, I like to quickly skim release notes, not scroll through automated patch updates from dependabot.

@deepu105
Copy link
Member

deepu105 commented Apr 7, 2022 via email

@deepu105
Copy link
Member

deepu105 commented Apr 7, 2022

@mshima @mraible I dont think the release page is used at all, since I see a lot of drafts. @pascalgrimaud do you know who set up the release drafter? there are too many drafts and the date is wrong as well

@pascalgrimaud
Copy link
Member

@deepu105 : there was a bug 1 month ago in GitHub, that's why there are a lot of duplicated drafts. All these drafts need to be deleted manually, then release-drafter can work normally

@deepu105
Copy link
Member

deepu105 commented Apr 7, 2022 via email

@pascalgrimaud
Copy link
Member

I know, I spent a lot lot lot of time to delete it in jhipster-lite project, but didn't have motivation to do it for generator-jhipster...

@deepu105
Copy link
Member

deepu105 commented Apr 7, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: bug 🐛 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ theme: reactive ⚛️ Spring WebFlux theme: security $500 https://www.jhipster.tech/bug-bounties/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants