PoC and Patching - Null Safety

We have strong evidence of three different null issues, two of which should be exploitable by us. Of course, there can always be unforeseen protections in place that prevent us from exploiting these bugs, so in this section, we will try to confirm their exploitability with a proof of concept.

Proof of Concept

Let us start with the first logic bug we identified with the token parameter. This bug should enable us to reset the password of any user by simply knowing their id, which is not necessarily considered to be a secret value and may be revealed by API calls or even in their profile URL.

Account Takeover

Let's target the local user we have, which has the id of 649f2893cba8d0d6e8412182. To perform this attack, all we need to do is to send a POST request to the /api/users/password/reset endpoint, and skip adding the token parameter in our request's body:

{
  "id": "649f2893cba8d0d6e8412182",
  "password": "123456"
}

If we send request, we get Password updated successfully!. If we attempt to login, we can successfully get in, which means that our first attack is successful:

Denial of Service

Excellent! Moving on with the second attack, and this time, we will also remove the password parameter from the above payload, and we don't have to include the token parameter to skip the check as we did above (or you may calculate the real token and include it, if you're feeling adventurous). Now, we will send the request to see how the application handles it:

We simply get no response back from the server. To check if this has caused any errors, we can go to the Docker tab in VSCode, right-click on the running container, and select View Logs, and will see that the application appears to have completely crashed. Indeed, if we go back to the browser and refresh, we see that the web application is no longer running:

With a simple request, we have completely taken the server down, and now no user would be able to access it, until it is manually restarted. Both of these examples show how serious null safety issues can be, and why we should take them seriously.

Patching

In this code, we have identified multiple issues, and even though we are not able to take advantage of all of them, we still need to apply patches for them all.

updateUserDetails

The first issue was in the updateUserDetails endpoint, and the root cause was in the UserSchema. We cannot simply require all parameters, as some endpoints need the schema validation, and do not need all parameters (e.g. login only needs email/password). At the same time, we cannot leave these parameters as optional for all cases.

So, the best solution would be to use conditional validation, and specify the cases in which we need these parameters to be required. For example, if we need the username parameter in UserSchema to only be required with the createUser and the updateUserDetails endpoints, and keep it as optional with the rest (as it is not being used there), then we can modify it as follows:

export const UserSchema = yup
  .object({
    useUsername: yup.boolean().default(false),
    id: yup.string(),
    name: yup.string(),
    username: yup
      .string()
      .when("useUsername", ([useUsername], schema) =>
        !useUsername ? yup.string().required() : yup.string()
      ),
    email: yup.string().email().required(),
    password: yup.string().min(5).required(),
    registrationDate: yup.date(),
  })
  .required();

As we can see, we added a new boolean parameter useUsername with a default value of false, and if this parameter is set to true, it will trigger the when option to make the user parameter .required(). When we use the schema for validation and need to require the username parameter, we can do so as follows:

await UserSchema.validate({
  useUsername: true,
  username,
  email,
  password,
});

Since useUsername is set to true, then this would mean that the username parameter is required, and if a null value is passed into it, it will return an error. Doing optional parameter checks this way is much more secure and much more maintainable than scattering them throughout the endpoints, which may introduce issues in any of the endpoints.

If we were using the same schema solution with TypeScript, then all of that would be detected automatically. In that case, if we misuse it, like setting useUsername: true but there's a case where username would be null, then it would notify us during edit-time and prevent the compilation of the code.

resetPassword

As for the resetPassword endpoint, we had multiple issues that we need to rectify. First, we need to ensure that all of the parameters are required, as they are only used once (in this endpoint), and all of them are needed, so there's no point in keeping any of them as optional. Keep in mind that requiring the entire object doesn't mean all of its parameters are also required, as we have shown in the previous section. So, we can fix the passwordResetSchema schema as follows:

const passwordResetSchema = object({
  // validate mongodb object id
  id: mixed((value) => ObjectId.isValid(value))
    .typeError("Invalid id")
    .required(),
  // validate bcrypt hash
  token: string()
    .matches(/[0-9a-f]{32}/i, "Invalid token")
    .required(),
  // validate password
  password: string().min(5).required(),
}).required();

Once this is done, we would know that the token will not be null during the execution of the code, so we will not have to test for that when comparing it to hashedToken. So, we can simply do:

if (token !== hashedToken) {
  return next({
    message: "Invalid password reset token.",
    statusCode: 403,
  });
}

Furthermore, as the function already uses strict inequality (!==), then this would also check for null and undefined, as it would not be equal to the type of hashedToken, which is a string. We need to rectify the default to success logic bug that we noticed, even if we know that the token variable would never be null at that point, we need to have it patched as a matter of principle. Ideally, we should only continue if the tokens do match, and fail otherwise. This may look something like this:

if (token === hashedToken) {
  try {
    const salt = await bcrypt.genSalt();
    const newHashPassword = await bcrypt.hash(password, salt);
    // update user password
    <SNIP>
  }
  <SNIP>
} else {
  return next({
    message: "Invalid password reset token.",
    statusCode: 403,
  });
}

Finally, we must always wrap any lines that use optional/nullable parameters with a try/catch block, like the lines that calculate the new password hash, which caused a DoS, as we have seen in the previous section:

else {
  try {
    const salt = await bcrypt.genSalt();
    const newHashPassword = await bcrypt.hash(password, salt);
    // update user password
    <SNIP>
  }
}

Of course, with the updated schema, we know that the password parameter would never be null, but this is done as a demonstration of how we should do it in that case. With that, we should have patched all of the parameter logic bugs found within our web application. Or did we? Try to see if you can find any others, in addition to whatever the exercises are testing.