Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(security): prevent arbitrary file write in SQL editor (CVE-2024-10835) #2268

Conversation

haawha
Copy link
Contributor

@haawha haawha commented Jan 2, 2025

Description

This PR fixes a critical security vulnerability (CVE-2024-10835) in the SQL editor API that could allow attackers to perform arbitrary file write operations through DuckDB SQL queries. The vulnerability could potentially lead to Remote Code Execution (RCE).

Key changes:

  • Added database type validation to identify DuckDB connections
  • Implemented comprehensive dangerous operation filtering for DuckDB
  • Added SQL execution timeout protection (30s)
  • Added file path pattern detection to block file operations
  • Improved logging for security events
  • Enhanced type conversion safety

How Has This Been Tested?

The security fix has been tested with:

  1. Normal SQL Operations:
  • Standard SELECT queries
  • JOIN operations
  • Aggregate functions
  • Sub-queries
  1. Security Tests:
  • File operation attempts via DuckDB
  • System command execution attempts
  • Various SQL injection patterns
  • Path traversal attempts
  • Whitespace bypass attempts
  • Timeout verification

Test Environment:

  • Python 3.8+
  • Latest DuckDB version
  • Both Linux and Windows environments

Snapshots:

Include snapshots for easier review.

Checklist:

  • My code follows the style guidelines of this project
  • I have already rebased the commits and make the commit message conform to the project standard
  • I have performed a self-review of my own code
  • I have commented my code with clear English comments
  • I have made corresponding changes to the documentation
  • Security review has been completed
  • All tests pass successfully
  • No existing functionality is affected

…0835)

Block dangerous DuckDB operations to prevent arbitrary file write and potential RCE.
@github-actions github-actions bot added the fix Bug fixes label Jan 2, 2025
Aries-ckt
Aries-ckt previously approved these changes Jan 5, 2025
Copy link
Collaborator

@Aries-ckt Aries-ckt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@haawha
Copy link
Contributor Author

haawha commented Jan 6, 2025

Hello, @Aries-ckt @sbabybird @AlphaHinex @xudafeng

How can my pr request be merged? Right now it needs to be approved by 2 reviewers to be automatically merged.

Also, #2269 is another pr I submitted.

Copy link
Collaborator

@fangyinc fangyinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@fangyinc fangyinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution, there is still a small problem, please format codes, you can run make fmt to format it.

@haawha
Copy link
Contributor Author

haawha commented Jan 6, 2025

Thank you for your contribution, there is still a small problem, please format codes, you can run make fmt to format it.

Thanks for the heads up. It's done and pushed.

Copy link
Collaborator

@fangyinc fangyinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@Aries-ckt Aries-ckt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aries-ckt Aries-ckt merged commit d40e801 into eosphoros-ai:main Jan 6, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants