File Attachments Corrupted when mbstring Overloading is Enabled

[Rick Gabriel]

When mbstring overloading is enabled in the php.ini file, uploaded files become corrupted (or incomplete) when downloaded. Eventum appears to be calculating the size of the file by using the strlen() function. This function is transparently replaced with mb_strlen() when mbstring overloading is activated. Instead of returning the number of bytes in the file, the number of characters is returned instead. Eventum then stores and uses an incorrect value for file size.

My recommended fix is to not depend on strlen() to determine file size. Instead, use mb_strlen() with "8bit" encoding if available. Alternatively, Eventum could use the filesize() function on the temporary file created for the upload.

if (function_exists('mb_strlen')) {
    $size = mb_strlen($string, '8bit');
} else {
    $size = strlen($string);
}

[Elan Ruusamäe (glen666)]

would be a lot easier if you pointed to actual code or method name

[Elan Ruusamäe (glen666)]

mbstring.func_overload safe strlen added

https://github.com/eventum/eventum/commit/864c855a1e978f9297ea39fc5f51a0bfeca65e58

waiting for you to point to exact code to modify, patch or PR in github welcome of course (or even bzr branch in launchpad)

[Rick Gabriel (klaxian1)]

Sorry, I'm not as familiar with the Eventum code as you. One example I found was in lib/eventum/class.attachment.php:507 (addFile()). I suggest doing a grep for "strlen" on the entire codebase. When you find cases where you intend to count bytes instead of characters, replace with your new wrapper function. If you are operating on a file instead of a string, I suggest the PHP filesize() function instead. I wish I could be more help.

As a workaround, I updated my Apache configuration to prevent overloading string functions with mbstring in Eventum's directory. However, this PHP setting can only be done at an Apache system level (ie. not htaccess). Therefore, some users may not have that ability.

[Elan Ruusamäe (glen666)]

Do you have url where download was corrupted? i could look up what's the code behind that.

[Rick Gabriel (klaxian1)]

Sorry, my installation is for my company and it's private. However, you can reproduce the issue by enabling mbstring function overloading (set to 7) in your php.ini and attaching any file with binary data (ie. JPG, PDF, etc.) to a ticket. TXT files with single-byte character sets will not produce the problem.

I apologize if I'm not providing enough information to resolve this, but I can't understand what more I could do besides patching it myself. strlen() is not the appropriate function to determine the number of bytes in a file nor string. It works in many cases, but not all. Every instance where Eventum is using strlen() to calculate the number of bytes should be replaced with your wrapper function or filesize() when dealing with files. I have given you one example file/line number in the attachment class, but there may be others.

[Elan Ruusamäe (glen666)]

doh, no i don't want direct url, just sample of url, anything that's below eventum/htdocs is public and common to all installations.

[Elan Ruusamäe (glen666)]

do you realize, i don't just want to blindly fix something, i want to actually test that there was problem and the fix fixed. do you have at least sample file to attach to this issue that causes the issue?

[Elan Ruusamäe (glen666)]

and actually you *can* fix this yourself and send your .patch or Pull-Request (github) or Branch (launchpad)!

[Rick Gabriel (klaxian1)]

I'm not sure what more I can offer. Every binary file and text file with a multi-byte character set causes the problem when mbstring overloading is enabled in php.ini. When a file is uploaded, Eventum calculates and stores the number of bytes in the file with strlen(). However, this is incorrect when mbstring overload is enabled. When a user later downloads the file, the HTTP size header is set too low so only part of the file is actually downloaded - resulting in an incomplete or corrupted file. For an example, you can use ANY binary or unicode file. I have explained how you can test and reproduce the issue. You don't need a specific file from me. If you still don't understand or you are not willing to fix the problem, please close the ticket because there is nothing more I can provide. Thanks.

[Elan Ruusamäe (glen666)]

i finally got around for testing this.

configured php.ini:

mbstring.func_overload = 7

i've uploaded text file, with unicode chars in it.
dropzone uploader showed it correctly being 13 bytes
eventum before patching showed 10 bytes in issue attachment listing
eventum after patching showed 13 bytes

however, clicking view file or download file gave me full file of both uploads (the 10 byte and 13 byte)

so were your downloads actually corrupted? or just showed wrong size only?

so i'm not able to reproduce corruption for now. maybe you can attach file here which i can test better?

[Elan Ruusamäe (glen666)]

and download file uses database value ($file['iaf_filesize']):
https://github.com/eventum/eventum/blob/v3.0.3/htdocs/download.php#L52

the 10 bytes upload test:
http://localhost/~glen/eventum/download.php?cat=attachment&id=281

13:12:52 mysql{4}> select iaf_id,iaf_filesize from eventum_issue_attachment_file where iaf_id=281;
+--------+--------------+
| iaf_id | iaf_filesize |
+--------+--------------+
| 281 | 10 |
+--------+--------------+
1 row in set (0.00 sec)

but http headers show 13:

Cache-Control:must-revalidate
Connection:Keep-Alive
Content-Disposition:inline; filename="k%C3%A4la.txt"; filename*=UTF-8''k%C3%A4la.txt
Content-Length:13
Content-Type:text/plain

how!?

[Elan Ruusamäe (glen666)]

my only guess is that "php engine" somehow fixes up that i issue wrong content-length header, but later echo out full 13 bytes:

https://github.com/eventum/eventum/blob/v3.0.3/lib/eventum/class.attachment.php#L101