Signatures implementation has some flaws

[AlexMedia]

I've recently started using ImageFlow for a new project, and I'm very happy with it. I especially like the signatures feature, which prevents people from abusing ImageFlow and potentially DoSsing my server. However, I think there are some flaws in its implementation.

Signature verification is optional Assume the following configuration in Startup.cs:

app.UseImageflow(new ImageflowMiddlewareOptions()
                 .SetMapWebRoot(true)
                 .SetRequireRequestSignature(false)
                 .AddRequestSigningKey("MySigningKey"));

With this configuration, the signature is only verified if one is provided in the first place. If the signature element is not provided in the query string, verification does not take place at all.

If you configure ImageFlow like this you might as well not bother with signatures altogether, as "Mr. Hackerman" can still instruct ImageFlow to generate a 64K image.

Access to raw images is blocked when signatures are required Assume the following configuration in Startup.cs:

app.UseImageflow(new ImageflowMiddlewareOptions()
                 .SetMapWebRoot(true)
                 .SetRequireRequestSignature(true)
                 .AddRequestSigningKey("MySigningKey"));

With this configuration, ImageFlow requires that a signature is provided and verifies that the signature is as is expected. However, when a request is made without any querystring parameters it gets blocked.

Take the following bit of CSS:

div.header {
    background-image: url('/img/header.png');
}

With the configuration as above, ImageFlow blocks the request originating from the stylesheet and returns 403 Forbidden to the client. This makes it almost impossible to reference images from a stylesheet, as the encryption key might change per environment.

Therefor, I'd like to propose the following changes to the way signatures are handled:

• If a request signing key is set in configuration, ImageFlow always enforces a signature to be present and valid when a request is received with query string parameters.
• If a request is received without any querystring parameters, don't block the request unless SetRequireRequestSignature is set to true.

[lilith]

Thank you for the detailed and thoughtful issue.

A few notes: Querystring parameters aren't the only thing that could be used in a DOS attack; S3 and Azure and other blob providers could also be overwhelmed causing excessive HTTP traffic. We might need an even more subtle approach to support CSS.

There are also cases where request signing is used to protect something else, like S3/Azure access, rather than querystring commands.

I'm not sure what the idea solution/design would be, but I will be thinking about it. Please do share any comments or thoughts.

[AlexMedia]

I am not sure how signatures help to prevent excessive requests being made to AWS or Azure. After all, there is nothing stopping an attacker from simply requesting the same image to be loaded a thousand times when they have a URL with a valid signature (which can be obtained easily). As long as the signature is valid, the request will be executed. Only caching on the application level will help to mitigate this.

I think the signature behaviour should be as follows (in pseudocode):

if len(request.querystring) == 0:
  if SetRequireRequestSignature == false:
    serve_the_original_image()
  else:
    return_forbidden()
else:
  if calculate_signature(request.querystring) == request.querystring["signature"]:
    serve_the_processed_image()
  else:
    return_forbidden()

Ideally, it should also be possible to configure the behaviour on path level.

This way it would be possible to serve application assets (such as a logo) in the original size, while access to user uploaded content can be restricted. This would prevent people from getting access to the original sizes of user-uploaded content.

[lilith]

Do you mean make SetRequireRequestSignature only apply to non-processed files?

What do you think of the following API?

.SetSignatureRequirement("/", StringComparison.OrdinalIgnoreCase, 
  SignatureRequired.ForAllRequests, new string[] { "signing key a", "signing key b"})
.SetSignatureRequirement("/logos/", StringComparison.OrdinalIgnoreCase, 
  SignatureRequired.ForQuerystringRequests, new string[] { "signing key c"})

I'm wondering how to handle case sensitivity on the prefixes too. Should I just force users to make a choice? Unlike .MapPath, inconsistencies in case sensitivity will have security consequences, particularly if people don't use the most restrictive options for "/".

It's also worth noting that rewrite rules can completely bypass request signing protection if they pull values from custom querystring fields or from the path itself - and if ForQuerystringRequests is used.

[AlexMedia]

The proposed API looks good to me, it would cover all my use cases.

When it comes to case sensitivity, I think it would be preferred to have the same behaviour as used by MapPath and its cloud storage equivalents.

[lilith]

Unfortunately the behavior for directories on the filesystem depends on the OS configuration which isn't readily available.

On Thu, Oct 29, 2020, 7:42 AM Alex notifications@github.com wrote:

The proposed API looks good to me, it would cover all my use cases.

When it comes to case sensitivity, I think it would be preferred to have the same behaviour as used by MapPath and its cloud storage equivalents.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/imazen/imageflow-dotnet-server/issues/26#issuecomment-718760829, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH4M34NCXT2FIZSFWWLSNFWNDANCNFSM4SWMEWVQ .

[lilith]

I'm now leaning toward this, so that the user is forced to define a default signature required setting for the root.

.SetSignatureOptions(new RequestSignatureOptions(SignatureRequired.ForAllRequests, new string[] { "signing key a", "signing key b"})
.ForPrefix("/logos/", StringComparison.OrdinalIgnoreCase, 
  SignatureRequired.ForQuerystringRequests, new string[] { "signing key c"}));

[AlexMedia]

I like the idea of setting a default on root level, and overriding it on a lower level.

[lilith]

It's now implemented in 0.4.0: https://github.com/imazen/imageflow-dotnet-server/releases/tag/v0.4.0