MSSQL relies on vulnerable version of Tedious

[ashutosh-015]

The version of Tedious is vulnerable and thus needs to be downgraded.

Expected behavior:

there must not be any high vulnerabilities

Actual behavior:

Tedious is vulnerable as it relies on the wrong version of jsonwebtoken which has high vulnerabilities.

Configuration:

downgrade the tedious to 11.X

Software versions

• NodeJS:
• node-mssql:
• SQL Server:

[dhensby]

The version of tedious is not going to be downgraded from 15 -> 11 in a major version and I'm not going to release a major version to degrade the experience.

Whilst the jsonwebtoken may be flagged as vulnerable it actually needs to be exploitable to be a problem, have you reviewed the relevant code in the dependency chain to determine if there is a realistic attack vector here?

I'd be highly surprised if these vulnerabilities have any impact on the usage of tedious because we aren't providing any authentication in the library (ie: we aren't taking in JWTs, evaluating them, and then providing access to resources on the outcome of the JWT). I imagine that the JWT library is only used to create JWTs and thus, it would be for the SQL Server to correctly validate them and approve/deny access.

If you can provide a concrete example of where the vulnerabilities are exploitable, I'll be happier to take this more seriously.

[cawoodm]

Whilst it may not be actively exploitable - though that is hard to prove/disprove - and I understand your reasoning. Many of us routinely scan our code for vulnerabilities and stuff like this block our CI/CD process.

The recommended fix by npm audit is to downgrade mssql to 7.2.0 link which kinda sucks.

It seems the issue is not directly with mssql but with a deeper package (tedious):

• "mssql": "9.0.1"
  • "tedious": "^15.0.1" (15.1.2)
    • "@azure/identity": "^2.0.4"
      • "@azure/msal-node": "^1.10.0"
        • "jsonwebtoken": "^8.5.1" !!!

[cawoodm]

Posted to Tedious.

[ashutosh-015]

@cawoodm Thanks for taking it into concern and understanding the POV. My next resort definitely was to go to Tedious cause even that depends on the jsonwebtoken. Thanks for posting it there.

[dhensby]

Blindly following NPM audit advice without actually performing a risk analysis is a misuse of npm audit. Pretending that vulnerabilities only exist if disclosed, and treating all vulnerabilities as exploitable whether they are or not is not helpful. All software comes with risk/vulnerabilities regardless of the disclosure of security issues and npm audit shouldn't be used as a comfort blanket to make developers feel safe. If the legitimacy of the vulnerability isn't evaluated, then how can we trust the disclosed issue at all?

I've commented on this before (see #1288 (comment)) but there is no objective for this library to have a clean npm audit output, but there is an objective to have the library be secure.

For projects that are not using azure logins via JWTs, they're not vulnerable, if the library is not validating JWTs, they're not vulnerable, if the @azure/msal-node library is correctly implementing their JWT validation they're not vulnerable. So the first thing to do is to actually decide if this vulnerability applies and, if it does, fix it (open a PR to @azure/msal-node).

Don't ask me to downgrade to v11 of tedious which may simply remove that notice, but what new ones will it bring up and why do would the older dependencies in that version be any more secure than the modern ones and why is it worth the performance degradation (feature losses) and huge "upgrade" pain to all the mssql users just to keep your CI green because you won't evaluate vulnerability disclosures on a case-by-case basis?

This isn't aimed specifically at OP; these kinds of issues get posted every so often and I have the same feelings; if it can't be proven that the vulnerability is exploitable, then it's not a vulnerability.

[dhensby]

Whilst it may not be actively exploitable - though that is hard to prove/disprove

I dispute this is hard to verify:

1. GHSA-8cf7-32gw-wr33 - flaw with verify logic
2. GHSA-27h2-hvpr-p74q - flaw with verify logic
3. GHSA-hjrf-2m68-5959 - flaw with verify logic
4. GHSA-qwph-4952-7xr6 - flaw with verify logic

So - where is jsonwebtoken used in the library - it's used by @azure/msal-node. Let's take a look at the library in question...

https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/msal-node-v1.14.5/lib/msal-node/src/client/ClientAssertion.ts - this file is the only place that jsonwebtoken is used and it is only used once on line 110 and it is only used to sign a JWT. There is no expoitable vulnerability. QED

This took me... 3 minutes to check.

[ashutosh-015]

Whoa! @dhensby I understand your opinion and the fact that you must be responding to so many issues, as far as the suggestion is concerned, I did not raise this issue so that you blindly follow me or downgrade immediately. I was running through our project and fixing the vulnerabilities before our prod push. We faced one issue, and I felt it would be worth sharing to see if this is already in notice/ if it has something that I might not know of.

[dhensby]

@ashutosh-015 understood. As I said, it's not directly aimed at you, it's a summary of why npm audit outputs need to be interrogated and not just blindly followed.

Thanks for clearing up that you didn't expect me to downgrade the tedious version, but your initial issue does say:

The version of Tedious is vulnerable and thus needs to be downgraded.

So hopefully you can understand that from my perspective it looks like I'm being asked to degrade the library for a non-exploitable vulnerability.

I've tried to be thorough and clear with my reasoning on why I won't be taking any action (the crux of it being there is no exploitable vulnerability) whilst also hoping to inform and educate others on the pitfalls of relying on npm audit as a vulnerability scanner.

Vulnerabilities don't start and end with npm audit, it is just a tool to help get you some signals on publicly disclosed vulnerabilities.

[ashutosh-015]

@dhensby, sounds pretty good to me, and yeah!! I am sorry for misplacing the word needs in the description and using a definitive tone. Thanks for letting me know, and yeah Happy coding!

[cawoodm]

I would agree with dhensby that we mortals do blindly follow npm audit. Ignoring npm audit would, in general, be a worse idea.

I'm just not clear how to proceed, we have a security gate, I don't see a means to say: scan for vulnerabilities, please ignore these known non-issues.

[dhensby]

I don't deny it's a crappy situation, you're getting an alert about a vulnerability that doesn't apply. This is one of the flaws discussed here: https://overreacted.io/npm-audit-broken-by-design/

It really depends on the levels you're working to. In my professional capacity I used to use Snyk (I am not endorsing or recommending it, I'm just stating fact) and the advantage it had was that it allowed you to acknowledge vulnerabilities and set a period of time for the issue to be ignored, that did come with the disadvantages that Snyk is so horrendously expensive.

If your CI breaks on audit, then I don't really have any advice for you beyond change your CI because, as we are learning here, these results are advisory only. Personally, I'd use audit to raise warnings but not block CI, those warnings can then be assigned to team members to investigate and resolve.

It seems that the only other solution is to use the --json flag with audit and manually parse it in tandem with some custom logic around ignored vulnerabilities (like Snyk can). This proposal is not really moving: npm/rfcs#18, but would be what we want.

Unfortunately, this dependency is deeply nested (lots of sub-dependencies in the way), so pushing for new releases is the final option, but there's a big dependency tree to get updated so it will take time.

While writing this at looking at the npm RFC I saw there is this package which may help but I cannot vouch for it https://www.npmjs.com/package/npm-audit-resolver

[dhensby]

This issue should be resolved once the next release of @azure/msal-node is published (AzureAD/microsoft-authentication-library-for-js#5473)

[cawoodm]

Looks like @azure/msal-node has just been released so we're good again. :-)