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

JS: Remove some FPs from the hardcoded-credentials query #16417

Merged
merged 4 commits into from May 7, 2024

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented May 3, 2024

Fixes #16360 and #16359.
Replaces #16244

I looked at what credentials-kinds we have, and I found the below (list is possibly non-exhaustive):

"user name"
"password"
"authorization header"
"key"
"token"
"credentials"
"Node.js http(s) client login username"
"Node.js http(s) client login password"

I changed the last two to just user name and password.

We already filtered away keys that look like dummy passwords for password, credentials, and token, and it seemed reasonable to just add key to that list.

I also added a note in the QHelp with two sample passwords that we recognize as dummy-passwords.

Evaluations (nightly, default) look reasonable.
It removes keys that are obviously not meant to be actual secrets or the values are used for tests.
And performance is unaffected.

Copy link
Contributor

github-actions bot commented May 3, 2024

QHelp previews:

javascript/ql/src/Security/CWE-798/HardcodedCredentials.qhelp

Hard-coded credentials

Including unencrypted hard-coded authentication credentials in source code is dangerous because the credentials may be easily discovered. For example, the code may be open source, or it may be leaked or accidentally revealed, making the credentials visible to an attacker. This, in turn, might enable them to gain unauthorized access, or to obtain privileged information.

Recommendation

Remove hard-coded credentials, such as user names, passwords and certificates, from source code. Instead, place them in configuration files, environment variables or other data stores if necessary. If possible, store configuration files including credential data separately from the source code, in a secure location with restricted access.

If the credentials are a placeholder value, make sure the value is obviously a placeholder by using a name such as "SampleToken" or "MyPassword".

Example

The following code example connects to an HTTP request using an hard-codes authentication header:

let base64 = require('base-64');

let url = 'http://example.org/auth';
let username = 'user';
let password = 'passwd';

let headers = new Headers();

headers.append('Content-Type', 'text/json');
headers.append('Authorization', 'Basic' + base64.encode(username + ":" + password));

fetch(url, {
          method:'GET',
          headers: headers
       })
.then(response => response.json())
.then(json => console.log(json))
.done();

Instead, user name and password can be supplied through the environment variables username and password, which can be set externally without hard-coding credentials in the source code.

let base64 = require('base-64');

let url = 'http://example.org/auth';
let username = process.env.USERNAME;
let password = process.env.PASSWORD;

let headers = new Headers();

headers.append('Content-Type', 'text/json');
headers.append('Authorization', 'Basic' + base64.encode(username + ":" + password));

fetch(url, {
        method:'GET',
        headers: headers
     })
.then(response => response.json())
.then(json => console.log(json))
.done();

Example

The following code example connects to a Postgres database using the pg package and hard-codes user name and password:

const pg = require("pg");

const client = new pg.Client({
  user: "bob",
  host: "database.server.com",
  database: "mydb",
  password: "correct-horse-battery-staple",
  port: 3211
});
client.connect();

Instead, user name and password can be supplied through the environment variables PGUSER and PGPASSWORD, which can be set externally without hard-coding credentials in the source code.

References

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@erik-krogh erik-krogh merged commit 8b91914 into github:main May 7, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive for jsonwebtoken.sign with a dummy password used as a secret key
2 participants