fix: clean password by using url object itself

[DEMON1A]

Note

I was asked to open a PR here because it doesn't seem to have that significant risk to npm, but it's best practice to fix this issue, So I just took a copy of the report reported to GitHub Security Team at HackerOne with ID 1917565, that's the full details for the issue including the fix for this issue

Description:

An invalid replacement has been discovered on npm-cli function replaceInfo that's responsible for removing passwords from URLs getting passed to npm, to disallow it from being stored and saved on the logs exposing users passwords since there's many custom private registries at the time that big companies uses

Normally the format of HTTP basic authentication credentials is http://username:password@hostname/path, the replaceInfo function is just replacing the password text with **** four stars by default every time no matter the length of the password, But it hasn't been any sort of validation on the order of the replace function, that means if the basic authentication credentials is the same meaning the username and the password is the same admin:admin the replaceInfo function gonna replace the username as the first item instead of replacing the password, showing and confirming for sure the password for that host confirming it's the same as the username.

The code is located inside npm-registry-fetch/lib/clean-url.js, and the vulnerable code is located here

const cleanUrl = (str) => {
  if (typeof str !== 'string' || !str) {
    return str
  }

  try {
    const url = new URL(str)
    if (url.password) {
      str = str.replace(url.password, replace)
    }
  } catch {
    // ignore errors
  }

  return str
    .replace(tokenRegex, `npm_${replace}`)
    .replace(guidRegex, `npm_${replace}`)
}

The problem with your code is that you're replacing the original str that's usually the full URL trying to remove the parsed url.password object with replace object, and the String.prototype.replace() function in javascript normally has a max replace of 1 that would end up replacing the username of every case

I have modified and written a patch for this issue, Instead of replacing the original str variable and giving a chance for the max replace to happen, or implementing an algorithm to keep track of the replaces, I just rebuilt the URL using the url object setting url.password = replace then building the url using str = url.toString(), That's a 100% safe solution for this issue and I have tested it already

Here's the new code for clean-url.js

const { URL } = require('url')

const replace = '****'
const tokenRegex = /\bnpm_[a-zA-Z0-9]{36}\b/g
const guidRegex = /\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b/g

const cleanUrl = (str) => {
  if (typeof str !== 'string' || !str) {
    return str
  }

  try {
    const url = new URL(str)
    if (url.password) {
        url.password = replace
        str = url.toString()
    }
  } catch {
    // ignore errors
  }

  return str
    .replace(tokenRegex, `npm_${replace}`)
    .replace(guidRegex, `npm_${replace}`)
}

module.exports = cleanUrl

Should I open a PR on https://github.com/npm/npm-registry-fetch to save you the time?

Steps To Reproduce:

1. Use any npm command that requires the registry like npm ping --registry=http://admin:admin@localhost/
2. You will notice that the first admin ( username ) is the one getting replaced
3. Update clean-url.js as I mentioned and try the same command again
4. You will notice the password is the one getting replaced this time

Cheers

Impact

Basic authentication credentials disclosure in certain cases where the username is the same as the password confirmed by the invalid protection method

[wraithgar]

This is definitely a code quality cleanup and not security issue. It's a pretty hard sell to convince me that if your username and password are identical, then logging them in a file is somehow a greater security risk than already exists by having a username and password that are identical.

It is definitely better to let the url object replace the password, much less prone to breaking and much more declarative as far as what the code is doing.

[DEMON1A]

Yeah you actually got a point, it doesn't seem to actually show a significant security impact but trust me it would make the process much easier if you could just identify if the username is the same as the password from the first look, some people might just spend hours brute-forcing till reaching this point of knowing the password, brute-forcing might not be even possible so it's a pretty low/none risk here, it's all about the process here ig

This is definitely a code quality cleanup and not security issue. It's a pretty hard sell to convince me that if your username and password are identical, then logging them in a file is somehow a greater security risk than already exists by having a username and password that are identical.

It is definitely better to let the url object replace the password, much less prone to breaking and much more declarative as far as what the code is doing.