2 forms submitted the same time leak data

[FransdeJong]

If 2 people submit a form at the same time, person A receives data of person B and person B receives no email.

All personal data of person B is send to person A and that is a major leak of confidential data.

Tested on:

• Umbraco Cloud
• Umbraco 8.5.3
• Umbraco Forms 8.3.1

If more data is needed please let me know

[nul800sebastiaan]

Yikes 😲

How can we test "at the same time", any good way to reproduce? If not, how did you get to this conclusion please?

[FransdeJong]

Just called with the client.

They filled in the form on two pc's and counted down to push on the send button
I'm looking up the timestamps in the database right now.

The workflow that is used is "send email" a standard workflow. No custom code is used

[FransdeJong]

Timestamps:

2020-02-20 10:56:02.680
2020-02-20 10:56:02.697

[FransdeJong]

Anything else you need to know?

[clausjensen]

@FransdeJong Thanks for providing instructions. This definitely sounds very strange.
One last thing I'd like you to verify is whether the actual submitted data (which should be stored inside Forms in the backoffice) is also messed up or if that is separated correctly between the two submits.

[FransdeJong]

@clausjensen No problem at all. In the backoffice the data is like it should be.

[clausjensen]

@FransdeJong Which workflow are they using for sending emails?
Plain, razor or xslt?

[FransdeJong]

The "Send Email" flow. That is the "just data" email

[clausjensen]

Thanks... crap. 😄 I had an idea.. now that's shot down.
@Shazwazza @bergmania could you pitch in here?

SendEmail is a workflow .. it is being added via the TypeLoader based on inheriting from WorkflowType:

composition.WithCollectionBuilder<WorkflowCollectionBuilder>().Add(() => composition.TypeLoader.GetTypes<WorkflowType>());

Doing this - could this cause a single instance of this workflow to be registered and reused across multiple requests?

I'm not too familiar with the Forms codebase, but I cannot find where we actually ever set the Message field of a SendEmail instance (it is a public field on that class).... but I'm thinking that it may be due to something being reused and the Message field actually being set by request#2 before request#1 is done sending.

I'm however still having a hard time seeing how the Email field wouldn't also be changed at this point - which would then cause the body#2 to be sent to recipient#2 twice instead of sending body#2 to recipient#1 and body#2 to recipient#2

[FransdeJong]

Can you reproduce the issue?

[clausjensen]

@FransdeJong Yep.. can reliably reproduce the issue now (100% of the time). It's a pretty nasty concurrency issue happening due to how workflows are implemented/used and how things are registered lifetime-wise in the container.
Not really sure there's a pretty solution to the problem but I have one that at least works now.
Will need to be reviewed first though.

[FransdeJong]

Is there a solution to this bug? One of our client is really concerned and want this fixed before they can add new forms.

[alanmac]

I'd like to make a friendly suggestion that this issue gets put on a fast track to resolution. With it being confirmed as reproducible, it is a vulnerability that can be exploited in the wild now and, depending on the context of the site being targeted, could result in a serious breach of personal data & privacy, with a potentially high risk for the individuals concerned. Please make sure it gets the attention it deserves as soon as possible.

[clausjensen]

@FransdeJong @alanmac this issue is already escalated and currently in review.

[alanmac]

Good to know @clausjensen, thanks.

[nul800sebastiaan]

Sidenote: it's incredibly difficult to reproduce in a production environment and I see no way to use it for an actual attack to grab a user's Form submission. You'd basically need to sit next to them and do a countdown as Frans detailed in his repro steps.

[FransdeJong]

That is partly true. Our client encountered this issue in a production environment where forms are used a lot. That's when they started testing. So the countdown was just for the reproduction scenario. It happens once a week at the moment. Depends on how much traffic the form gets.

I don't think there's a risk for a attack, but it is very likely that a site is going to leak personal data since the furthest apart I have found until now was almost 10 seconds between submissions.

Therefor in my opinion this issue should be treated as a big risk.

[clausjensen]

@FransdeJong @alanmac
I can't say when this will be released yet, but I've made a custom build of 8.3.1 where this is fixed so you can use that for the clients that are concerned about this.
UmbracoForms.Files.8.3.1-alpha.224.zip

[FransdeJong]

Thanks, Are the changes only in a Dll? If so, can you tell me which one? Makes it easier to copy the files in the project.

[clausjensen]

@FransdeJong Yep - only dll if compared to 8.3.1. It's both the .Core and .Core.Providers.dll. You can skip .Web.

so others who might want to apply this - please make sure to update everything if you are not on 8.3.1

[nul800sebastiaan]

Therefore in my opinion this issue should be treated as a big risk.

Yep! Was just trying to say I don't see a possibility for a targeted attack like Alan suggests.

[FransdeJong]

@clausjensen Thanks!

@nul800sebastiaan Than we're on the same page. Misread your comment.

[alanmac]

Yep! Was just trying to say I don't see a possibility for a targeted attack like Alan suggests.

So I may be missing something here @nul800sebastiaan, but given the scenario that opened this issue:

If 2 people submit a form at the same time, person A receives data of person B and person B receives no email. All personal data of person B is send to person A and that is a major leak of confidential data.

If Person A (with bad intentions) submits form data on an affected website repeatedly (automated) with the intention of intercepting the details of others who are submitting forms on the same website (which may be highly trafficked and expect regular form submissions), could this not be exploited?

Depending on how the email workflow is configured, it may include all (or even some of the submitted data) submitted by Person B being sent to Person A. Now as an example, what if that website is:

• A mental health service, collecting special category data
• A Government website, collecting sensitive data
• Financial-service based site, collecting high risk data
• A pharmacy, collecting prescription data that can reveal health issues

[nul800sebastiaan]

@alanmac You're 100% right. When I hear "targeted" I am thinking of a single known target, not random people who happen to be on this website at this time as well.

[alanmac]

Ah, I'm thinking of 😈 targeting many 😇 😇 😇

[nfarrell]

Why was this ticket closed? Is it still a valid issue or has it been rectified?

[FransdeJong]

The workflow is not really transparent for forms issues. The issue is not fixed for end-users but it is fixed internally so it could be released in a future release.

In the meantime you can use a alpha build that's posted in this thread when you are running the latest version.

I think it would be more clear if issues in this tracker would only be closed when a fix is released.
It would be even better if there was a "included in version x" tag so we know it when it is planned.

[nfarrell]

Thanks @FransdeJong, that clears things up. I guess it's the lesser of the two evils - do I use the version with an obvious and reproducible data leak or use an alpha version of the lib in production site. Thanks again!

[nul800sebastiaan]

Yes of course we should always add a release label so you know in which version this is (going to be) fixed. Apologies for missing that on this one. Human error! 🙈

[nul800sebastiaan]

I don't have a release data for 8.3.2 yet, but the alpha should be pretty safe to use, I don't think there's anything else changed in it since 8.3.1 except for this bug fix.

[FransdeJong]

No problem at all. This clears things up a lot.
I would advice to release this update asap. On sites with heavy forms use it's impossible to tell where data will be sent and not everyone reads the tracker.

[TimGeyssens]

Hey, does this only effect sites running on Umbraco v8 (so with the composition stuff) or could this also be an issue on v7 sites? @clausjensen

[nul800sebastiaan]

This specifically was an issue in v8 with workflows being registered as Singletons. It does not affect v7.

[TimGeyssens]

Ok thanks for the confirmation

[TimGeyssens]

And tend to agree with @FransdeJong that a non alpha release for folks that don't follow the progress on the issue tracker (and don't know about the fix) would be much appreciated... even contacting them pro-actively to tel that they might be affected... since I think most countries (EU at least) have a Data breach notification obligation law (so the law is that they must report a possible data leak within 72 hours)

[ronaldbarendse]

So the problem is caused because the same WorkflowType instance in reused (as its registered as singleton) and it's storing state in fields/properties (in this case the MailMessage), right?

As the workflow settings are configured using attributed properties, that would mean those could change during the execution of the workflow (when the workflow is used on the same or even other forms with different settings), especially when the execution takes some time to complete (like sending an email or posting to a URL).

Besides fixing the registration part, it would be a great enhancement if the workflows used virtual methods with all required objects/state as parameters. That way, any developer can reuse the built-in email workflow and alter the generated HTML, MailMessage properties (add Reply-To header, BCC, etc.) or method of sending (SMTP, API, etc.)! This would also allow better garbage collection, as the properties currently stay allocated/referenced for the lifetime of the instance (if not manually disposed/finalized).

[TimGeyssens]

@ronaldbarendse sounds good but off topic (and this issue is already closed), maybe create a new feature request issue?

[ronaldbarendse]

@TimGeyssens I've already mentioned this on another (still open) issue: #51 (comment). This would also make #139 possible.

[TimGeyssens]

@ronaldbarendse sweet, some golden oldies there ;) hope they get some attention