[JENKINS-75914] Do not mask text less than 2 characters#377
Conversation
The bug submitter was using a zero length secret text and it significantly increased the log output. Require that the matched regular expression pattern must contain at least two characters. Testing done: * Wrote automated tests to confirm original exception is returned when string is less than 3 characters * Ran the revised plugin in Jenkins and confirmed that the excess asterisks are not displayed
|
Just out of curiosity - what is the value/purpose of using 4 (constant) number of replacement characters? Wouldn't 1 do just fine? |
I believe that the choice of "****" as the replacement text is for the convenience of the person reading the log. It is easier to detect a sequence of characters than a single character. One of the stated goals of the pattern used to match the masked text is that it should prefer the longest match. By using 4 characters, we don't disclose the length of the matched text and we still provide enough text that a person scanning the log file is likely to see the "****" string as indicating sensitive information that has been masked. |
| if (pattern.toString().length() < 3) { | ||
| return unmasked; | ||
| } |
There was a problem hiding this comment.
possibly doesn't work when you have bound multiple credentials (and one is empty?) as the pattern will be built from all of them and still match?
I would rather suggest that the pattern/masker is not even created for an empty credential / match.
Additionally why 2 characters here - is that different to what is masked in the logs? is a user warned in the credential plugin when creating a that a single character password will not be masked?
There was a problem hiding this comment.
I wonder if a different heuristic (fallible rule) would be a better choice. If the size of the masked exception message is more than 10% larger than the original exception, then we refuse to mask the original exception. That would replace the heuristic that uses regex pattern length with a heuristic that tries to avoid the specific case that was seen at Eclipse where the size of the logged exception message was much larger than the original exception message.
What do you think of that alternative?
There was a problem hiding this comment.
wonder if a different heuristic (fallible rule) would be a better choice. If the size of the masked exception message is more than 10% larger than the original exception, then we refuse to mask the original exception. That would replace the heuristic that uses regex pattern length with a heuristic that tries to avoid the specific case that was seen at Eclipse where the size of the logged exception message was much larger than the original exception message.
What do you think of that alternative?
I am wondering why the console log did not blow up before this change (ie we previously still masked the console log, the security change just added masking of an exception). Is there a reason we can not apply the same logic, perhaps @Kevin-CB recalls?
If the regular console masks a single character (which I believe it does) then I would say the masked exception should also do the same - the whole point of this code is to behave the same and mask what would have been masked in the console to prevent leakage.
A single character secret is insecure and should not be used (you can extrapolate that to small length too), and would create interesting masked console output.
- TBH not sure why an empty file is any better but well - that should not cause any masking just like in the console.
I would say that heuristics shouldn't be used, we just need the behaviour to match the console.
There was a problem hiding this comment.
There was a problem hiding this comment.
I am wondering why the console log did not blow up before this change (ie we previously still masked the console log, the security change just added masking of an exception). Is there a reason we can not apply the same logic, perhaps @Kevin-CB recalls?
There is a check for empty credentials for regular messages on the console log, which wasn't implemented for exceptions.
Right. In regular case indeed is better with I was under the influence of the result with empty pattern, which gives
Got it. Thanks. |
I agree with |
4 participants
[JENKINS-75914] Do not mask text less than 2 characters
The bug submitter was using a zero length secret text and it significantly increased the log output. Require that the matched regular expression pattern must contain at least two characters.
Testing done
Submitter checklist