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

This commit fixes a potential denial of service vulnerability in logrus.Writer() that could be triggered by logging text longer than 64kb without newlines. #1376

Merged
merged 3 commits into from
May 15, 2023

Conversation

ozfive
Copy link
Contributor

@ozfive ozfive commented Mar 10, 2023

Fix denial of service vulnerability in logrus.Writer()

The related issue #1370 explains the problem.

This pull request fixes a potential denial of service vulnerability in logrus.Writer() that could be triggered by logging text longer than 64kb without newlines. Previously, the bufio.Scanner used by Writer() would hang indefinitely when reading such text without newlines, causing the application to become unresponsive.

To fix the issue, I've modified the writerScanner() function to split input into chunks of up to 64kb using a custom split function. I've also set the scanner buffer size to the maximum token size using bufio.MaxScanTokeSize.

With these changes, logrus.Writer() now properly handles long, newline-free log messages without causing a denial of service.

Note that the code has also been updated with more comments and that this fix is backwards-compatible. It does not affect existing applications using logrus.Writer()

…us.Writer() that could be triggered by logging text longer than 64kb without newlines. Previously, the bufio.Scanner used by Writer() would hang indefinitely when reading such text without newlines, causing the application to become unresponsive.
@ozfive
Copy link
Contributor Author

ozfive commented Mar 13, 2023

@sirupsen Its not clear to me what is failing in the tests as I don't have any experience with appveyor. Can you take a look and see what the issue is?

@dolmen
Copy link
Contributor

dolmen commented Apr 17, 2023

This commit fixes a potential denial of service
vulnerability in logrus.Writer() that could be
triggered by logging text longer than 64KB
without newlines. Previously, the bufio.Scanner
used by Writer() would hang indefinitely when
reading such text without newlines, causing the
application to become unresponsive.
@ashmckenzie
Copy link
Contributor

ashmckenzie commented May 4, 2023

@ozfive I believe the following patch is required for this to pass CI:

diff --git a/writer.go b/writer.go
index 36032d0..7e7703c 100644
--- a/writer.go
+++ b/writer.go
@@ -75,7 +75,8 @@ func (entry *Entry) writerScanner(reader *io.PipeReader, printFunc func(args ...
                if len(data) > chunkSize {
                        return chunkSize, data[:chunkSize], nil
                }
-               return 0, nil, nil
+
+               return len(data), data, nil
        }

        //Use the custom split function to split the input

Also, I think what @dolmen is trying to say is that your commit message could be a little more concise on the first line and then move the more detailed content to the 'body' section.

I've created ozfive#1 to demonstrate the fix as well as a suggested commit message. WDYT?

Also, I created #1382 as a Draft, just so CI can run.

@ozfive
Copy link
Contributor Author

ozfive commented May 4, 2023

You are a god among men. I am still learning git so will make it my job to look up patching and try to understand what you did here.

@ashmckenzie
Copy link
Contributor

@sirupsen I believe this is ready for review, when you have time please 🙂 The git commits might need a squash though.

@scarroll32
Copy link

@sirupsen is it possible to trigger the CI on this? Would love to see it merged.

@voidspooks
Copy link

@sirupsen Hi, would it be possible to get this change merged?

@sirupsen sirupsen merged commit 6acd903 into sirupsen:master May 15, 2023
@ashmckenzie
Copy link
Contributor

Thanks so much for merging @sirupsen 🙇‍♂️ May we please have a new release to include this fix? 🙏

@sirupsen
Copy link
Owner

ah yes tagged v1.8.3

@ashmckenzie
Copy link
Contributor

ah yes tagged v1.8.3

Thanks for creating a new tag @sirupsen 🙇‍♂️ The latest tag/release prior to the new v1.8.3 tag was v1.9.0 (which is what we use in our golang projects here at GitLab). Will there also be a v1.9.1 tag and release?

@sirupsen
Copy link
Owner

🤦🏻 v1.9.1 released, I didn't pull down the most recent tags before deciding the new one

@Luap99
Copy link
Contributor

Luap99 commented May 17, 2023

Note that the code has also been updated with more comments and that this fix is backwards-compatible

This is no longer splitting at newlines at all so it is chaining the output for users. With this change there is no reason to use the Scanner at all, you could just Read from the reader and write that with printFunc().

Anyway the reason I am here is because the change is broken and causes panics: #1383

sirupsen added a commit that referenced this pull request May 17, 2023
This reverts commit 6acd903, reversing
changes made to e59b167.
svetasmirnova added a commit to percona/percona-toolkit that referenced this pull request May 19, 2023
svetasmirnova added a commit to percona/percona-toolkit that referenced this pull request May 29, 2023
sirupsen added a commit that referenced this pull request Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants