Fix security issues in copilot chat app#1090
Fix security issues in copilot chat app#1090DavidParks8 wants to merge 26 commits intomicrosoft:mainfrom
Conversation
samples/apps/copilot-chat-app/webapi/Auth/PassThroughAuthenticationHandler.cs
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/Controllers/SemanticKernelController.cs
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapp/src/components/views/BackendProbe.tsx
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/DocumentImportController.cs
Outdated
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/ChatHistoryController.cs
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/Auth/ChatOwnerAuthorizationHandler.cs
Show resolved
Hide resolved
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<AskResult>> InvokeFunctionAsync( |
There was a problem hiding this comment.
This controller is a generic controller to invoke SK functions. It should not contain any chat related components.
There was a problem hiding this comment.
Yet, it must contain the minimum components required to validate that the user is allowed to pass specific context variables. Otherwise, users will be able to invoke skills for chats they do not own. Validation needs to be here as well as within skills as part of a defense in depth strategy.
There was a problem hiding this comment.
This controller will not be used for any chat related features. Maybe we need to be explicit about it.
There was a problem hiding this comment.
Regardless of what it will be used for in the app, it still allows for remote execution of the chat skill if a bad actor constructs the proper request. This must be guarded against.
When it comes to backend security, the frontend's default usecase is not relevant. What one could do when calling the backend is what matters. We see people take tokens out of browsers and automate attacks against backends all the time.
There was a problem hiding this comment.
Totally agreed! Need further discussion with @adrianwyatt and @hathind-ms
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/ChatController.cs
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/DocumentImportController.cs
Outdated
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/DocumentImportController.cs
Outdated
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/DocumentImportController.cs
Outdated
Show resolved
Hide resolved
|
Thanks for your PR!! Just a note for our team, this PR will create a lot of conflicts with the multi-user feature branch. In the multi-user setting, some chat-related authentication logic may need to change from this PR. For example, the data schema for ChatSession will be different in the multi-user setting. We need to talk about which one we want to let in first. |
|
@TaoChenOSU, This PR needs to go in first because if it had been filed via MSRC, your team would have been required to drop everything to fix it immediately. Security takes precedence over new features. I chose to instead save your team all that trouble and, with the goahead from @adrianwyatt, implement it. |
I am looking at this today and the review will likely bleed into tomorrow. Thank you for your awesome work on this! |
| @@ -7,33 +7,26 @@ namespace SemanticKernel.Service.Options; | |||
| /// <summary> | |||
There was a problem hiding this comment.
We have two stories here:
- A generic service for SK as a service.
- A copilot chat that is built on top of the generic service.
We would like the Copilot chat components to be segregated from the generic service code to make it relatively easy for people to remove Copilot chat from the service.
Can we keep the original class and create the new one called ChatAuthenticationOptions within the CopilotChat dir?
There was a problem hiding this comment.
Such segregation is not recommended or encouraged due to the associated risks.
By separating the Copilot chat components from the generic sk service code, it opens up the possibility of invoking any registered skill without proper validation. This could potentially lead to unauthorized access and misuse of the system. Maintaining a cohesive codebase ensures that necessary checks and balances are in place to validate and authorize skill invocations, thus safeguarding the integrity of the system.
Instead of segregating the components, I would suggest focusing on implementing robust security and validation mechanisms within the codebase. This way, you can ensure that only authorized and validated skills can be invoked, mitigating the risk of potential vulnerabilities. It is crucial to prioritize system security and maintain a unified codebase for better control and protection.
If you were to bring this to a security review, I would tell you to delete the sk controller entirely because it is an avenue for remote code execution vulnerabilities. The reason I did not delete it in this PR is because I was attempting to close the riskiest security issue fast and first.
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/ChatController.cs
Show resolved
Hide resolved
samples/apps/copilot-chat-app/webapi/CopilotChat/Controllers/ChatController.cs
Show resolved
Hide resolved
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| public async Task<ActionResult<AskResult>> InvokeFunctionAsync( |
There was a problem hiding this comment.
This controller will not be used for any chat related features. Maybe we need to be explicit about it.
|
Is there anything additional I need to perform before launching the web API and the web app besides the current instructions documented in the ReadMe? The web API throws errors when the web app tries to retrieve chat sessions. I have tried both pass through auth and AAD auth. |
|
Thanks again for the PR and especially outlining the concerns. The team is meeting this week to discuss how to mitigate the issues in line with our near-future planning. We'll be referencing this PR and will send you the plan as an FYI as soon as we have one. |
|
@DavidParks8 and I had a great real-time conversation and we'll be bringing in these changes piecewise over the next week or two. Closing this one in light of a set of incoming PRs. |
Motivation and Context
Description
api key auth was removed because it was not possible to be secure in a user context. Any user was able to operate on any other user's chats. A new auth policy was introduced to validate userIds coming from tokens rather than request bodies.
Contribution Checklist