encode user_id when using it in urls #296
Comments
|
Given that this would be a breaking change would we ever consider something like adding an |
|
I don't think that makes sense. If you're adding a parameter, just encode the value yourself. We'll fix this in the next major. |
Are you referring to the suggestion from @mmaddex or the need to encode values prior to using them in a URL? The fact that the API client library doesn't properly escape values prior to using them in a URL is both a bug and, possibly, a security issue. |
You're breaking abstraction.
The I suspect that a user of the library that doesn't URL-encode
What you're doing isn't made right by any quantity of existing usage. A broken implementation isn't justified by being operational 99% of the time, especially when it's a potential security issue like this. It's Auth0's job, as a vendor, to fix this, even if it's hard. I'm not fond of how you're suggesting it's my burden -- as a customer, FOSS developer, or security engineer -- to find a solution to a problem that Auth0 created. If adding proper URL escaping breaks existing implementations, it's a sign that the implementations and library were already mishandling data. |
I fully agree with you.
I'm asking our security team to go over this so we can assess the security implications of this bug.
I'm sorry you feel this way. That wasn't my intention at all. I was merely asking for any ideas you might have on how to fix this without breaking everyone else that is already encoding the parameter.
Again, I agree with you. That's why this issue is still open. Rest assured this will get fixed. While I don't have any estimates on this, I'm sure we'll get this sorted out in the near future. |
|
@davidstrauss Your other points aside, if I'm understanding you correctly, this is not a security issue. From a security perspective, it is never the responsibility of an untrusted client to send properly encoded, escaped data to the server. The burden is on the server to properly sanitize incoming data before using it. If a vulnerability exists here, then there's nothing stopping someone from just not using the client library and exploiting it anyway. Edit: After thinking about it some more, I might have misunderstood you. I can see how this can lead to some weird cases on the client side. Completely hypothetical -: endpoint does not exist where |
I agree, but I'm not sure how this applies to the situation. A "rule" in Auth0 ought to be able to trust this library to properly escape data before it reaches an HTTP request to the API. Rules may handle user ID values derived from something like a SAML assertion. A rule may then call the API (via this library) to read or update data for the user in the process of logging in. Because this library doesn't properly escape the value, it may be possible to trick the rule into updating a different user by sending a user ID with a URL delimiter or escaping that will decode to a different user ID. Here's a case where it would definitely be a security issue. Let's say we've annotated the user profile with fields listing additional constraints to that user's authentication. Perhaps it's a 2FA or region requirement. If the user profile isn't found because of lack of escaping, we may not apply those constraints. We also couldn't just refuse login on lack of a profile, as first-time logins may not have one yet. |
|
Yeah, I get you now. I misunderstood where you were coming from at first. I can definitely understand how this can be a security risk for people using the |
Thank you for clearing up what you're requesting, but please consider how you present such requests in the future. Combined with asking if it was even a bug, it was hard to read it as anything other than resistance to addressing the issue. Here is what I would suggest:
|
|
@davidstrauss Thanks for the thoughtful suggestion. I'll keep it in mind when fixing this. I also talked with a couple of coworkers and we had another idea as well. I'll work with the data team tomorrow to find if we have any users with a if (!user_id.includes('%')) {
user_id = encodeURIComponent(user_id);
}If this works, there's no extra overhead for anyone already encoding the user_id. In the next major, we can just document that we'll always encode it by default in the changelog. If it doesn't work, I'll use something similar to your suggestion so we move forward with the fix. Thanks for the patience! |
|
@luisrudge Activating encoding only when there's no percentage sign would cause slashes and other special characters to remain (potentially) exploitable. A malicious |
|
To provide a more actionable alternative, I'd encourage escaping when the provided Slashes may not be the only character that allows tricking rules into accessing the API in a way that reads or manipulates a different user than the active one, though. |
|
@davidstrauss sorry, I forgot to update you on this.
I've been talking with the security team and co workers and we got to the same conclusion.
I'll evaluate this with the security team as well. My idea for a minor version was to decode the user_id recursively until it's a "raw" string again (to prevent an attacker for encoding a string twice, for example) and compare the end result with the provided user_id.
Then, in the next major, I remove that fix, always encode the param and document the change. I think that's going to work. I'll write some test cases tomorrow to make sure I'm not missing anything |
A malicious If you're looking for a way to test for the string being already encoded, I would count the percent signs in the provided |
|
@davidstrauss Thanks again for all your patience on this. After pairing with @rikaardhosein, we came up with this solution: function containsUnsafeChars(s) {
const safeChars =
"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ$-_.+!*'(),%";
return !!s.split("").find(c => !safeChars.includes(c));
}
const maybeDecode = url => {
if (containsUnsafeChars(url)) {
return encodeURIComponent(url);
}
return url;
};
test("encodes regular user_id", () => {
const normalId = "auth0|1234";
expect(maybeDecode(normalId)).toBe("auth0%7C1234");
});
test("encodes a malicious user_id", () => {
const maliciousId = "anotherUserId/secret";
expect(maybeDecode(maliciousId)).toBe("anotherUserId%2Fsecret");
});
test("encodes a malicious user_id with a percentage", () => {
const maliciousId = "anotherUserId/secret%23";
expect(maybeDecode(maliciousId)).toBe("anotherUserId%2Fsecret%2523");
});
test("does not encode an already encoded user_id", () => {
const maliciousId = "auth0%7C1234";
expect(maybeDecode(maliciousId)).toBe("auth0%7C1234");
});This mitigates the risk of allowing a malicious id to hit a different endpoint. This is our proposed short-time solution for this issue (until a new major version is released). We still need to do some internal review of this, but I wanted to be transparent on what are our plans for this and also gather any feedback you might have on this solution. Thanks again! |
|
@luisrudge Thanks for continuing to follow up. I believe the proposed solution fixes the primary issues I raised. There remain ways to manipulate the encoding behavior, but I think this gets things to the the best possible place without altering the library's API. Most importantly, it prevents an attacker from supplying a |
|
Released in 2.18.1 |
Thank you for your work here! |
No description provided.
The text was updated successfully, but these errors were encountered: