search

cambiatus / backend

Cambiatus GraphQL API
GNU Affero General Public License v3.0
20 stars 18 forks source link

Privacy/security enhancement #262

Closed MatheusBuss closed 2 years ago

MatheusBuss commented 2 years ago

What issue does this PR close

Closes no issue

Changes Proposed ( a list of new changes introduced by this PR)

MatheusBuss commented 2 years ago

@lucca65 this should deal with most of the security concerns raised by sobelow. The only one remaining refers to the upload controller, which I'm not sure how to solve. Maybe we could discuss this one a bit better.

lucca65 commented 2 years ago

Oh i've also removed it from draft state, hope you don't mind. It was already looking ready

MatheusBuss commented 2 years ago

The only thing I think its missing is adding sobelow to our CI, making our builds fail if new security issues arise

Didn't realize that mix check was different from the CI :sweat_smile:. Should be working now.

MatheusBuss commented 2 years ago

It's failling because of the upload controller. I'm fairly certain that we can ignore this one but I wanted to double check it with someone.

It happens because on lib/cambiatus_web/controllers/upload_controller.ex - line 14 there's a File.read! and this function could be used to read other files in the production machine.

However I don't think that this attack is possible. The file beign read is created by Phoenix as a temp file when an image is uploaded (and the user has no input about this). So this function can only run when a file is uploaded and then Phoenix generates the temp file. The question is if there's no other way this function can be executed. On the tests I ran no code runs if something other than a file is passed to the api.

Another potential concern is steganography.

lucca65 commented 2 years ago

Pois é. O ideal é buscar por outras maneiras de fazer isso. Se não houver, podemos ignorar a analise: https://github.com/nccgroup/sobelow#false-positives

Como isso tá no CI agora, os Merges vão ser bloqueados se não passar. Ou seja, não é possível simplesmente aprovar o PR e ignorar, como dev tens que entregar uma build que é aprovada pelo CI.