corecfg: pam_env can only handle 1024 byte #4424
Conversation
| if err != nil { | ||
| return err | ||
| } | ||
| if len(s) > 1024 { |
There was a problem hiding this comment.
Is is the number of bytes that we need to track? I suspect you want the UTF-8 encoded byte array that you will then measure instead of the string. Some malicious input could get us to be under 1024 characters and way over 1024 bytes.
There was a problem hiding this comment.
len([]byte(s)) should do the trick
There was a problem hiding this comment.
len([]byte(s)) is always len(s). Maybe you meant len([]rune(s))? But that's not right (or rather, it's correct if s is known to be valid utf8).
If the input should be valid UTF-8 we should've checked as it came in, with e.g. utf8.ValidString (otherwise the string is just a fancy immutable []byte). Assuming we did that, and then wanted to limit the output to 1024 runes, utf8.RuneCountInString would do the thing without copying the data around.
There was a problem hiding this comment.
Looks like len() returns the byte count already. I wrongly assumed that you need to []bytes(.), but it's not necessary. If we're only interested in byte count, the code should be ok as it is.
Just for the reference, len("⌘") is 3 but len([]rune("⌘")) is 1.
There was a problem hiding this comment.
Ah, that's ... unexpected to say the least.
Still +1 on the patch.
There was a problem hiding this comment.
@zyga unexpected is what happens when you range over a string :-)
| if err != nil { | ||
| return err | ||
| } | ||
| if len(s) > 1024 { |
There was a problem hiding this comment.
len([]byte(s)) should do the trick
| restore := release.MockOnClassic(false) | ||
| defer restore() | ||
|
|
||
| tooLong := strings.Repeat("x", 1025) |
There was a problem hiding this comment.
Given the UTF-8 comment above, it probably makes sense to try with strings.Repeat("日", 400) as well.
Codecov Report
@@ Coverage Diff @@
## master #4424 +/- ##
=========================================
+ Coverage 78.04% 80.04% +2%
=========================================
Files 449 453 +4
Lines 30907 34713 +3806
=========================================
+ Hits 24121 27787 +3666
- Misses 4774 4822 +48
- Partials 2012 2104 +92
Continue to review full report at Codecov.
|
|
This needs some careful investigation of pam_env.c - i.e. what happens if we are over this limit |
|
Closing for now until pam_env.c got investigated. |
In bug https://bugs.launchpad.net/snapd/+bug/1737332 it is pointed out that pam_env has a limit of 1024 bytes for the env vars. From our
snap set core proxy.{http,https,ftp}we write /etc/environment. So we need to honor that limit. Or we go a different route and do not write /etc/environment and only usecore.proxy.{http,https,ftp}for snapd itself. But that feels slightly odd and is also a change from the existing behaviour.