Hi Yang, Groza,
Yang,
Forgot that, about
i wonder why even admin lack some permission,
It's because webshell uploads are devastating. They easily lead to RCE.
If for some reason we face a post-authN attack, we can't trust any
authentication.
And SecuredUpload is already enough complex.
and the transaction rollback cannot recover.
SecuredUpload is used in many parts.
If an anomaly appears processes stop in different manners, could be improved.
Le 09/09/2024 à 12:27, Groza Danut a écrit :
Hi Yang,
I also got that error when trying to upload the image using the "Add
Additional Images" screen in Catalog -> Products -> (Select a product) ->
Content.
When using the "Upload Image" in the bottom part of "Override Simple
Fields" screen. Not sure why we get this.
See just below
Hi Jacques,
Option 1
Is not going to work because checkMaxLinesLength throw an
exception(java.nio.charset.MalformedInputException: Input length = 1) at
line:
- List<String> lines = FileUtils.readLines(file, Charset.defaultCharset());
So it never gets to check the line length.
The error we get is
java.nio.charset.UnmappableCharacterException: Input length = 1
at
java.base/java.nio.charset.CoderResult.throwException(CoderResult.java:275)
at java.base/sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:326)
at java.base/sun.nio.cs.StreamDecoder.read(StreamDecoder.java:188)
at java.base/java.io.InputStreamReader.read(InputStreamReader.java:177)
at java.base/java.io.BufferedReader.fill(BufferedReader.java:162)
at java.base/java.io.BufferedReader.readLine(BufferedReader.java:329)
at java.base/java.io.BufferedReader.readLine(BufferedReader.java:396)
at java.base/java.nio.file.Files.readAllLines(Files.java:3415)
at org.apache.commons.io.FileUtils.readLines(FileUtils.java:2665)
at
org.apache.ofbiz.security.SecuredUpload.checkMaxLinesLength(SecuredUpload.java:855)
at
org.apache.ofbiz.security.SecuredUpload.isValidFile(SecuredUpload.java:254)
Using Charset.defaultCharset (or null, which is the same, ie OS dependent) when
reading with
FileUtils.readLines(file,Charset.defaultCharset());
is a problem. The file may have been created using another Charset. I remember having searched bout that. But even
http://illegalargumentexception.blogspot.com/2009/05/java-rough-guide-to-character-encoding.html#javaencoding_autodetect is not a 100% solution. So
even for text files it can be a problem and according to above there is no complete solution.
Option 2
This should work. I'll attach the fix to the issue you mentioned.
Unfortunately ATM I don't have time to
further investigate this issue, but I will leave a comment to the issue
with the findings so far.
As you certainly know image files are also text files. That's why I did not dig further than isValidImageFile() so far. Anyway I tend to agree that
it's improbable (but possible) that a trick using a very long line would be used in an image file.
Also, quite important. When looking at the code, I could not create a unit
test for this case, because
SecuredUpload uses private static methods where it interacts with files,
thus to create an isolated
unit test for this case there are only 2 options:
1. Upload a file image in the test resources folder, that will be used in
this test.
Not really optimal, since the codebase will grow really large really fast,
if we put a lot of test resources inside.
2. Mock methods that we don't care about in this test case.
Since the methods are static, and private, we cannot mock them using
Mockito(already added as a test dependency).
The only option here to mock them would be to add PowerMockito as a test
dependency and use that.
I believe going forward we should either consider adding PowerMockito as a
test tool, or consider writing more
testable code, as I think having the ability to write easy and fast unit
tests are a must in order to maintain code stability,
and prevent introducing bugs with changes.
I totally agree, just not a priority for me at the moment (and I guess in the
futur).
Feel free to work on it with the suggestions you made which seem great.
I have seen, and tested a bit (with some "attacking" images), your PR834. It
sounds OK with me.
Before pushing I'll though see if we can't handle the "All" case using
isValidImageIncludingSvgFile()
Thanks
Jacques
On Sun, Sep 8, 2024 at 9:00 PM Jacques Le Roux<jacques.le.r...@les7arts.com>
wrote:
BTW, there is already a Jira for that:
https://issues.apache.org/jira/browse/OFBIZ-12639
Any help is welcome :)
Le 08/09/2024 à 17:03, Jacques Le Roux a écrit :
Hi Groza,
After facing several webshell uploads I made SecuredUpload.java as
secure as possible OOTB.
I see 2 options here:
* Increase maxLineLength in security.properties (could be unsecure, but
not that bad)
* Improve SecuredUpload by having a special treatment for Images at
line 209
HTH
Jacques
Le 07/09/2024 à 12:52, Groza Danut a écrit :
Hi,
Have you tried to add an image to a product? I get an error message
saying
type unsupported for security reasons, even if the file type is .jpeg.
When debugging I found that ProductServices.addAdditionalViewForProduct
is
called.
At line 1083: org.apache.ofbiz.security.SecuredUpload.isValidFile is
called
Inside SecuredUpload line 254: checkMaxLinesLength throws an error,
since
this is a jpeg file.
As far as I see it, isValidFile should not checkMaxLinesLength if
the fileType is IMAGE.
