Fix: Enhance JSON Deserialization Security - Mitigate TypeNameHandling Vulnerability

[sush-101]

Bug 30973440 and Bug 30973442
CodeQL issue: https://liquid.microsoft.com/codeql/issues/621b3860-9992-462a-8a9d-0a24593f51a5?copilot_promptid=E91B0CE9-0C1B-4AC2-8A46-33F49B67E058

This commit addresses a potential security vulnerability within our test code and infrastructure related to JSON deserialization by enhancing the type handling mechanism.

Issue:
The previous deserialization configuration was using TypeNameHandling.Auto in Newtonsoft.Json. TypeNameHandling.Auto allows for automatic deserialization of types based on $type metadata embedded in the JSON. If an attacker can control the JSON input, they can potentially inject malicious $type properties to instantiate arbitrary types, leading to Remote Code Execution (RCE) vulnerabilities. This is related to our test infrastructure, so the potential security impact does not include production code running on customers' devices

Fix Implemented:

To mitigate this risk, the following changes have been made:

1. Disabled Automatic Type Name Handling (TypeNameHandling.None):

   • The TypeNameHandling setting in JsonSerializerSettings has been explicitly set to TypeNameHandling.None.
   • This is because the serialized JSON in our use case does not include $type metadata. Setting TypeNameHandling.None ensures that automatic $type processing is completely disabled, further enhancing security.

2. Implemented Secure Deserialization with KnownTypes Whitelist:

   • Updated TypeNameSerializationBinder binder to utilize a KnownTypes whitelist, explicitly defining the set of allowed types that can be deserialized.
   • The deserializer is now configured to use this SerializationBinder, ensuring that only types present in the KnownTypes whitelist are permitted for deserialization. This significantly restricts the attack surface and prevents the instantiation of unauthorized or potentially malicious types.
   • This approach aligns with secure deserialization best practices and follows the guidance outlined in: https://liquid.microsoft.com/Web/Object/Read/MS.Security/Requirements/Microsoft.Security.SystemsADM.10010#Zguide
     and recommendation: Solution using custom ISerializationBinder: https://liquid.microsoft.com/Web/Object/Read/ScanningToolWarnings/Requirements/CodeQL.SM02211#Zguide

Tested the changes in the local:

References:

https://liquid.microsoft.com/Web/Object/Read/ScanningToolWarnings/Requirements/CodeQL.SM02211#Zguide
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2326
https://liquid.microsoft.com/Web/Object/Read/MS.Security/Requirements/Microsoft.Security.SystemsADM.10010#Zguide

Azure IoT Edge PR checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines and Best Practices

• I have read the contribution guidelines.
• Title of the pull request is clear and informative.
• Description of the pull request includes a concise summary of the enhancement or bug fix.

Testing Guidelines

• Pull request includes test coverage for the included changes.
• Description of the pull request includes
  • concise summary of tests added/modified
  • local testing done.

Draft PRs

• Open the PR in Draft mode if it is:
  • Work in progress or not intended to be merged.
  • Encountering multiple pipeline failures and working on fixes.

Note: We use the kodiakhq bot to merge PRs once the necessary checks and approvals are in place. When it merges a PR, kodiakhq converts the PR title to the commit title, PR description to the commit description, and squashes all the commits in the PR to a single commit. The net effect is that entire PR becomes a single commit. Please follow the best practices mentioned here for the PR title and description

[tanmay-yerunkar-ms]

Looks good to me

[damonbarry]

In the PR description, can you make it clear that this is in test code, so the potential security impact does not include production code running on customers' devices?