CVE-2023-33967: Fix the SQL Injection by haoel · Pull Request #330 · megaease/easeprobe

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

Conversation

The MySQL/PostgreSQL data checking could have the SQL injection problem, this PR tries to fix it by adding the quotes in SQL and escaping the quotes in configuration.

func EscapeQuote(str string) string {

type Escape struct {

From string

To string

}

escape := []Escape{

{From: "`", To: ""}, // remove the backtick

{From: `\`, To: `\\`},

{From: `’`, To: `\’`},

{From: `"`, To: `\"`},

}

for _, e := range escape {

str = strings.ReplaceAll(str, e.From, e.To)

}

return str

}

Choose a reason for hiding this comment

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

as a cornor case, abc’def will be escaped to abc\’def, is this a correct result?

Copy link

Contributor Author

Choose a reason for hiding this comment

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

It’s correct. The sql injection always needs a quote to close the previous statement. So escape the quote is key work to prevent injection.

haoel added this pull request to the merge queue

Apr 25, 2023

Hi @haoel, great to see the issue is addressed. Could you open a Github security advisory for the SQL injection vulnerability we found?

@oxeye-daniel , thanks for the reminder, we have open a github security advisory at: GHSA-4c32-w6c7-77x4

please help review and let us know if anything is incorrect, as this is the first time we open such an advisory, thanks.

Hi @localvar thanks a lot! No problem, you can go ahead and add me as editor for the advisory so I can suggest changes

Reviewers

localvar localvar approved these changes

samanhappy samanhappy approved these changes