Incorrect commits for CVE-2018-3713?

[eric-therond-sonarsource]

For CVE-2018-3713, the pre/post patch commits seem incorrect:

git checkout adc9223332f07c81aaaa063c6565011e5e64f44a
./angular-http-server.js --path ./example/
curl -v --path-as-is http://127.0.0.1:8080/node_modules/../../../../../../../../../../../../etc/passwd

The path traversal attack works (/etc/passwd is disclosed) on this commit.

git checkout b31cd24a558ca847e5dd204e73fd940e1e47fdd3
./angular-http-server.js --path ./example/
curl -v --path-as-is http://127.0.0.1:8080/node_modules/../../../../../../../../../../../../etc/passwd

The path traversal attack doesn't work anymore on this commit.

On hackerone, the vulnerability was reported on the Jan 25, 2018, the maintainer did a fix on Jan 27, 2018 and strengthened the fix several months later.

[esbena]

I think CVE-2018-3713 is quite tricky, and I vaguely remember going through a similar process to the one you describe.

So let me first state that I agree with your date matching for the initial fix, and I also think that patch was sufficient, since both relative and absolute paths seemed to be handled safely after the patch. But the path-sanitization used in that project is not 100% best practice, so I wouldn't be surprised if I am simply failing to see a bypass vector.

The reason I have settled for the later commit, despite the date matching, is that it is a path-sanitization patch that satisfies the version in the advisory for CVE-2018-3713: Patched versions: 1.4.5.

I would prefer that the data in this repository aligns with the official advisory information and that it does not diverge because of a lacking ability to spot bypasses.
But perhaps the advisory is slightly wrong in this case, and should list 1.4.4 as the patched version.

[eric-therond-sonarsource]

Humm I think I understand the history of vulnerabilities:

• CVE-2018-3713 was reported here on H1 on version < 1.4.3 and said as fixed ( .. were not allowed anymore) with version 1.4.3. But you were right it's not a valid fix.
• And so a bypass (.. are encoded to %2e%2e), with no linked CVE, was reported here on H1, on version < 1.4.4 and fixed with version 1.4.4. The fix is:

var safeFileName = path.normalize(possibleFilename).replace(/^(\.\.[\/\\])+/, '');
var safeFullFilename = path.join(__dirname, safeFileName);

• Again there is another bypass, not reported anywhere, but by analyzing the maintainer comment on this commit I understand that he changed the last previous line about path.join to:

// Insert "." to ensure file is read relatively (Security)
var safeFullFileName = path.join(".", safeFileName);

What I understand, I am not completely sure, is that the http server could be executed like that:
node angular-http-server.js --path public
above, only the files inside public folder are expected to be served but as __dirname returns the folder where the current script (angular-http-server.js) resides thus an attacker can access files outside the public folder, the maintainer tried to change __dirname by . which corresponds to the directory on where the node command is executed, to me it's even worse.

• And so again the maintainer completely change the path resolution some months later with commit 34d4bd0cd0f00c46db30855a8c4aabae27eb0ac8. This time, a more traditional fix was implemented: normalization of the path with path.join and startsWith to compare the normalized path to an expected directory.

Eric

[esbena]

Bravo!
What a mess this was.

---

So the takeaway is that we have two kinds of implementation problems:

1. classic path injection vulnerabilities with relative paths that are insuffiiently sanititized (attack strings contain .. or %2e%2e)
2. logic error where files outside the intended web-root are accessible (the unintentional web-root is . and __dirname)

And then we have the advisory for CVE-2018-3713 that says v1.4.5 is the safe version, despite the implementation still containing the "logic error` security problem, which finally is fixed by simonh1000/angular-http-server@34d4bd0. The next version bump after that commit appears to be v1.6

---

https://github.com/ossf-cve-benchmark/ossf-cve-benchmark/blob/f8938834deadde6ffcb2701a2fa62b0a846f0b1b/docs/benchmark-CVEs.md#complete-benchmark-cves has a rather puritan with of this problem:

postPatch exists and contains zero relevant weaknesses, meaning that the vulnerability has been fixed properly.

I think that the logic error counts as "relevant" in this case, so I think the complete solution is:

• change the postPatch to be simonh1000/angular-http-server@34d4bd0, with prePatch being simonh1000/angular-http-server@c1b677f.
• request that CVE-2018-3713 is altered to state that v1.6 is safe (I'll try a polite ping)

Do you agree?

---

Meta:
I expect that the CVE entry will become uninteresting for existing path-injection analyses, since they probably are capable of finding the classic path injection implementation error mentioned above, and less capable of finding the logic error of using the wrong web-root (perhaps this could motivate a new kind of analysis that is focused on what an appropriate web-root is).

[eric-therond-sonarsource]

I agree,

• I can update this PR (update commits for CVE-2018-3713 #118) with the new prePatch/postPath commits if you want?
• Thank you to request the update of CVE-2018-3713, today nothing is said about version 1.4.4 in the security advisory, and the recommendation Update to version 1.4.3 or later. is in conflict with Affected versions, it's another arguments to definitely update it.
• About a new kind of analysis, just detecting normalized path (path.join, normalize ...) followed by a comparison (startsWith, indexof ...) with an expected path is certainly a good indicator of a complete path traversal attacks protection.

[esbena]

I can update this PR (#118) with the new prePatch/postPath commits if you want?

Yes, please. We probably also need to change the weakness locations.

Thank you to request the update of CVE-2018-3713

I have put in a request.

[esbena]

The CVE update request is tracked separately. I am keeping this issue closed.

[esbena]

The CVE has been updated.