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

Fix potential sql injection through fs path #190

Merged

Conversation

japroc
Copy link
Contributor

@japroc japroc commented May 5, 2021

Hello!

I have found a potential sql injection through fs path. I can't evaluate a risk, but still send a PR.

Flow description:

  1. Unsafe path comes from https://github.com/dns-stats/hedgehog/blob/develop/src/DSCIOManager.cpp#L216. Potentially it may be something like a' or '1' = '1. This string will successfully inject in sql query further.
  2. This parameter goes to initialization of DSCDataManager. https://github.com/dns-stats/hedgehog/blob/develop/src/DSCIOManager.cpp#L235
  3. Then raw server_name is used in sql query. https://github.com/dns-stats/hedgehog/blob/develop/src/DSCDataManager.cpp#L307. The same happens with node: https://github.com/dns-stats/hedgehog/blob/develop/src/DSCDataManager.cpp#L341.

I added an escaping of unsafe character. But better fix is to use pqxx builtin escape functions like quote.
I was unable to compile the project. So i decided to make a safer fix.

Thanks.

@saradickinson
Copy link
Contributor

Hi there,

Thanks for this report - we appreciate your interest. You’ll have probably noticed that this project is no longer actively maintained (it is superseded by a different DNS-STATS product) but we’ve taken a look at your PR anyway.

The two parameters you identified are generated by reading in directory names on the ‘Data Manager’ server (as described in the User Manual: https://github.com/dns-stats/hedgehog/blob/develop/docs/Hedgehog_User_Guide.pdf). This server is where files from nodes are uploaded to, and so an attacker would need enough access to that server to be able to create a ‘dummy’ directory there in order to compromise those parameters. We don’t provide specific guidance on upload procedures but we do assume that the Data Manager server can only be accessed by authorised users. Because of this, we don’t believe this specific attack is possible without such a compromise of the Data Manager server.

A small aside is that the list of server directories that are processed for data import is limited to those that are already in the database (see https://github.com/dns-stats/hedgehog/blob/develop/tools/refile_and_grok.in#L165), although the list of nodes is not. Servers in the database are restricted to alphanumeric characters and underscores. An additional check could be added in that file to ensure the list of nodes processed is also limited to those already in the database.

However, since your proposed fix is straightforward and harmless, we have accepted it.

@saradickinson saradickinson merged commit 58922c3 into dns-stats:develop May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants