[Bug] Magic link abuse by authorized user

[tinti]

Hi,

I would like to report a potential abuse of magic links by a user with valid credentials. In short, I believe the current implementation restricts the number of magic links an unauthorized user can create but allows authorized users to create an unlimited number.

Creation logic

The following comment states that magic links are be limited over time.

https://github.com/aws-samples/amazon-cognito-passwordless-auth/blob/84fc375ce1177df45b371a89acdfee78c14aab45/cdk/custom-auth/magic-link.ts#L50-L51

The magic link creation code indeed limits the number of magic link over a period of time using DynamoDB conditions.

https://github.com/aws-samples/amazon-cognito-passwordless-auth/blob/84fc375ce1177df45b371a89acdfee78c14aab45/cdk/custom-auth/magic-link.ts#L254-L281

If a user attempts to request more than one magic link within MIN_SECONDS_BETWEEN without using the first one, the DynamoDB condition will fail.

Validation logic

Given a user and a magic link the validation implementation will try to get and remove the item from the DynamoDB table.

https://github.com/aws-samples/amazon-cognito-passwordless-auth/blob/84fc375ce1177df45b371a89acdfee78c14aab45/cdk/custom-auth/magic-link.ts#L354-L388

After getting and removing the magic link some validations are performed and the authentication either succeeds or fails. The fact is that if a (username, magic link) tuple is found in the DynamoDB table it will be removed.

Flaw

A malicious attacker which has valid users credentials can cycle through: request magic link, authenticate, logout, request magic link, authenticate, ...

graph TD;
    request_magic_link-->authenticate;
    authenticate-->logout;
    logout-->request_magic_link;

Since the validation logic removes the table entry, the creation logic condition will not deny a new magic link. Thus, more than one magic link can be created in less than MIN_SECONDS_BETWEEN by a valid user.

Possible solution

Instead of removing the entry use a logic exclusion (invalidate the entry but keep it at the table). This will require changes to accommodate more than one userNameHash and will increase the number of items in the table.

[ottokruse]

Thanks for the report @tinti

What's your take on the legitimacy of this scenario?

The throttle feature (1 magic link per min) is to counteract misuse and brute force attempts from threat actors.

The scenario you describe is basically for legitimate users to spam themselves. I'm not sure this threat scenario is legitimate or dire enough to warrant code changes to prevent it. Why would a malicious user do this?

It would make the code slightly more complex as well to prevent this. Not inclined to that unless we're convinced it's needed.

[tinti]

Hi @ottokruse,

Thanks for the reply. To be honest I believe this scenario is more feasible than likely.

But I can see this becoming a problem if a malicious user is trying to:

• disrupt the sign-in process
• increase the service bill

If the medium where the magic links are sent has limits the bad actor can force the service to reach such limits. For instance, the email provider could only allow 10000 emails/day or 100 emails/min. Beyond that the service will be partially impaired. Another case would be DynamoDB provisioned capacity. The bad actor can hit its limits too.

To increase the bill, each magic link creation involves DynamoDB operations, KMS operations and SES operations (or other email provider operations). Each of those will incur costs.

What's your take on the legitimacy of this scenario?

From a cost perspective I am concerned. A bad actor should not be able drive costs. More info below.

The scenario you describe is basically for legitimate users to spam themselves. I'm not sure this threat scenario is legitimate or dire enough to warrant code changes to prevent it. Why would a malicious user do this?

Imagine a service which allows users to sign-up. Then imagine a bad actor creating several bots. This can lead to a big bill.

It would make the code slightly more complex as well to prevent this. Not inclined to that unless we're convinced it's needed.

Given the problems that can happen I believe the complexity is recommended. Otherwise I would recommend at least a footnote or a comment that the MIN_SECONDS_BETWEEN only works for uncompleted sign-in attempts.

[ottokruse]

It's good to think about this and thank you for bringing it up.

Realistically I think the dynamo, kms and ses costs will be hard for a single user to rack up. You'd have to go through so much trouble for a few dollars.

Limits I would be more concerned with, as you can DOS others by making the system reach limits.

I'm gonna ask around for views on this.

WAF might also be a solution. Potentially with a rate based rule

[tinti]

If you believe it can be upstreamed I would be quite happy to implement a logic exclusion.

[ottokruse]

WAF could work.

However let's think about the implementation in code for a sec to see how much more complex it will be.

I think we need to change this line as you say: https://github.com/aws-samples/amazon-cognito-passwordless-auth/blob/84fc375ce1177df45b371a89acdfee78c14aab45/cdk/custom-auth/magic-link.ts#L357-L379

It should not delete the row, but rather set a new attribute "used" to true WITH a condition expression that this attribute must not yet exist, and that the attribute userNameHash does exist (ensuring this update is not an INSERT), in addition to the condition expression statement that is currently already in the Delete call (attribute_exists(#signatureHash) AND #signatureHash = :signatureHash).

Thinking that may actually be it. Don't think we need to change the DB schema. It can remain as is. Then this is less complex probably than adding WAF.

What's your take?

[tinti]

I agree.

This logic is quite similar to creating a logic exclusion. At the end of the day both solutions are: don't delete the key instead mark it as used.

The only thing I would double check is how the insert logic should handle all combinations of is_used and is_expired.

What do you think? I can work on this later.

[ottokruse]

Please have a go at it!

I checked it and the insert logic can remain unchanged as far as I could quickly tell. But please have a stab and be vigilant

[tinti]

Hi @ottokruse. You were right about the insert logic.

Please take a look at the WIP here: #176

[tinti]

Hi @ottokruse. I believe this can be closed. Thanks for accepting the patch.