opencrvs / opencrvs-core

A global solution to civil registration
https://www.opencrvs.org
Other
88 stars 70 forks source link

R05 Validate all incoming payloads #7502

Open rikukissa opened 2 months ago

rikukissa commented 2 months ago

For form data submission and basic events like declaring, we should postpone adding validation until custom events. For things like user creation however this should be implemented asap

[!WARNING] There's a chance to improve on input validations as part of #7052 where all new Event API fields should be properly secured with Zod schemas. As for form data validation, the new form data model will help us validate form inputs on the backend-size based on the form schema provided by the country. This does not however fix the validations that we are missing currently for instance in user creation.

Description

Security assessment details

Technical Overview

The application accepted values to be entered in fields where they were otherwise prevented due to a lack of server-side validation. When altering user details, the application used client-side validation to ensure that special characters were not used in certain fields. By manipulating the data sent to the server, this validation could be bypassed and prevented characters could be used.

Potential Impact if Exploited

Should prevented values be used in unexpected places, any part of the application or any third- party applications making use of the data may behave in an unexpected manner, resulting in incorrect data processing.

Recommendations

The application should perform complete data and input validation against expected values on the server as well as the front end before persisting the values to the back end.

Tech tasks

Step 1:

Validate the incoming payload to the public FHIRs APIs such as FHIR Locations to ensure no traversing is possible

Step 2:

Validate the incoming payloads for the create user form

Option 1

1) create packages/form-validations which generates a package @opencrvs/form-validations that the frontend and gateway can use 2) publish the package, so that country-config can use the type definitions 3) create a gateway function that can be used in the createBirthRegistration routes for example, that fetches forms and validates the corresponding fields. This can be difficult with our current flexible mapping ? Time to move into a Record<FormFieldIdentifier, string> type of a GraphQL-model? 4) import the shared functions from the form-validations package similarly as we currently do

Option 2

The first option can get difficult with mapping, so we could start with only validating the forms that are known to us. So handle the country specific forms later.

naftis commented 2 months ago

I believe this being low priority, as no specific security risk was found even though the validations aren't there. Inputting HTML-tags doesn't still render them as HTML tags so the security risk is low. Correct me if I'm mistaken, but is this a feature rather than a security fix?

rikukissa commented 2 months ago

This is ultimately a data hygiene question with also links to security. As we lack a proper holistic approach to incoming data validation, it's really just a matter of time before someone finds an exploit. The exploit could also be submitting a value not part of a dropdown select for instance or the client sending a record state not known to us. These things would cause data corruption and exceptions potentially blocking users from using the application. XSS is part of this but not the whole story.

naftis commented 2 months ago

Points to discuss this week with Tameem:

rikukissa commented 2 months ago

Do you think it would make sense to bring this a step closer to GraphQL? I'm thinking a lot of the request body shape & field validation happens currently by the GraphQL schema. Is there a way to leverage / extend that to cater for more expressive validations?

https://www.apollographql.com/blog/graphql-validation-using-directives This at least looks pretty decent

Or is it better to keep the validation separated for future where we might not use GraphQL?

naftis commented 1 month ago

That could be a good option for the forms we control. For the configurable validations in forms, we could do something similar for TEXT:

{
  ...
  backendValidation?: {
      pattern?: Regexp
      maxLength?: number
      format?: 'email' | '...'
  }
}

Then add the option validation for SELECT's as we should know all the possible options?

rikukissa commented 1 month ago

Yea 👍 That also made me think that maybe instead of type: 'TEXT' we could have type: 'EMAIL' etc

rikukissa commented 1 month ago

For forms, it might be easiest to implement this as part of #7052

euanmillar commented 2 weeks ago

@makelicious @naftis please note that this tech debt issue should be commenced as part of custom vital events work. @Nil20 @Zangetsu101 @tumbledwyer there will be work to do with user creation also.