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

[ozfive]

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()

[ozfive]

@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]

@ozfive https://www.freecodecamp.org/news/how-to-write-better-git-commit-messages/

[ashmckenzie]

@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 https://github.com/ozfive/logrus/pull/1 to demonstrate the fix as well as a suggested commit message. WDYT?

Also, I created https://github.com/sirupsen/logrus/pull/1382 as a Draft, just so CI can run.

[ozfive]

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]

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

[scarroll32]

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

[voidspooks]

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

[ashmckenzie]

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

[sirupsen]

ah yes tagged v1.8.3

[ashmckenzie]

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]

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

[Luap99]

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: https://github.com/sirupsen/logrus/issues/1383