Closed asarubbo closed 7 years ago
mdadams
commented
7 years ago This is not a bug. The BMP file claims to use palette with a few billion colors. This requires a large memory allocation (on the order of 64 GB), which fails. ASAN is just saying that we are out of memory.
asarubbo
commented
7 years ago Afaik this is a bug. Issues like this received a CVE because it is a memory consumption.
mdadams
commented
7 years ago The issue s this: To the best of my knowledge, ASAN always generates a warning/error if one requests a huge amount of memory that cannot be succesfully allocated. This is certainly the case on my machine. For example, the following completely correct code will generate an ASAN failure on my machine, due to the malloc failing as a result of insufficient memory:
int main() { char *p;
if (!(p = malloc(SIZE_MAX))) {
printf("failed\n");
exit(1);
}
free(p);}
Clearly, it is not the fault of the program that there is insufficient memory available to satisfy the malloc request. That is, there is no bug in this program. Similarly, there is no bug in jasper. You are giving jasper a corrupt JPEG file. In order to process this file, jasper needs to correctly allocate a huge amount of memory (which is assumed to be needed later) to handle the massive palette in the JPEG file. This malloc fails and jasper terminates gracefully due to being out of memory. At least, this is the case on my machine. From the output that you provided, it seems that the same thing is happening on your machine as well. So, unless you have some evidence to the contrary, I do not see any bug here.
asarubbo
commented
7 years ago mdadams
commented
7 years ago I am sorry but I meant "BMP file" not "JPEG" file above, as the test file was in BMP (not JPEG) format.
@asarubbo: The practical problem is that there is no good way to fix this problem without imposing arbitrary restrictions that would prevent certain legitimate uses of jasper on certain platforms. The above URL is basically saying that code should never allow someone to allocate too much memory. But "too much memory" is subjective. Where do I draw the line? JasPer is used on a very wide range of hardware (from small embedded systems to large supercomputers). Therefore, there is no good choice of a maximum memory allocation threshold. I could artificially impose arbitrary restrictions on the size of palettes and many other things, but this would easily preclude some legitimate uses of the code. Furthermore, if using too much memory is a genuine concern, there are much bigger fish to fry than this particular BMP issue. If someone really wants to impose memory usage restrictions, it is easy to do (e.g., modify malloc or jas_malloc, or use resource limits in the OS, etc.). It's just that I don't want to ram my own personal memory-usage policy choice (which would inevitably be a bad choice for some users) down the throat of every user. Does this make sense?
As best I can tell, the large memory request is not due to an overflow. It is due to a large amount of memory being necessary to do the job. If I am mistaken, and the large request should not be large (and is only large due to overflow), please let me know because in this case there is a bug that needs fixing.
asarubbo
commented
7 years ago While you are opening the image with the imginfo binary, if it goes out of memory, it is just an inconvenience. While there is an application which relies on jasper to parse some images, then there will be a Denial of Service.
So usually, people may not consider these type of bugs security relevant, but I never seen similar cases not considered as a bug.
However is not normal at all that jasper needs more that 64GB of ram to parse an image of 50 byte.
asarubbo
commented
7 years ago The solution for me is do more checks before allocate the memory.
mdadams
commented
7 years ago What should the check be? It must work for all possible file formats that JasPer supports now and might support in the future. Since compression can be extremely high in some special cases, you cannot assume that a small coded image file must always require only a small amount of memory to decode. Also, the approach must also work well for small embedded systems with only a few MB of memory and for massive machines with TB of memory. For these reasons, I don't think that the checking should be done in the codec themselves, which some of your comments seem to suggest. My point is not that there is no solution. It's just that things need to be thought about more carefully in order to arrive at a solution that works well for ALL users of JasPer.
Please understand that I am not saying that your comments are without any validity. It's just that I do not necessarily agree with the solutions that some of your comments seem to imply. I will give more thought to this matter, but please understand that I consider things like memory corruption and undefined behavior bugs much more serious than this particular issue. Maybe modifying the JasPer memory allocator to track total memory usage and then allowing the library user to set an upper threshold on memory use would address this issue. This is not something that can be handled correctly inside the code for the JPEG2000/JPEG/BMP/etc. codec themselves, however.
Anyways, I have made a note of this issue in my to-do list. Let's just leave it for now.
mdadams
commented
7 years ago As an experimental feature, I have added the ability to limit the total memory usage used by the JasPer library. See commit 65536647d380571d1a9a6c91fa03775fb5bbd256. You can try it out if you like. It is not enabled by default.
To enable this feature, build the code with something like: EXTRA_CFLAGS=-DJAS_DEFAULT_MAX_MEM_USAGE=1000000 ./configure --prefix=/tmp/jasper make clean && make Note: You need C11. Also, you probably don't want to set the detault to only about 1 MB like I did above.
mdadams
commented
7 years ago As an addendum to my previous comment, you can do things like: imginfo --memory-limit 1000000 < data/images/stawamuschief.pnm This overrides the default maximum memory usage set at build time.
I get a memory allocate failure on 1.900.5
Stacktrace:
Testcase: 2.crashes.zip