Potential Exif leak / JPEG wrong orientation

[papereth]

While looking into why some JPEG's orientation wasn't matching its thumbnail, I've noticed that the orientation-correction code in post.php does something odd. Specifically here:

if ($config['strip_exif'])
    $file['exif_stripped'] = true;

It's not actually stripping the exif, it's just setting $file['exif_stripped'] = true; which actually makes the code skip stripping the exif metadata later. And I'm fairly confident that this command:

$error = shell_exec_error(($gm ? 'gm ' : '') . 'convert ' .
        escapeshellarg($file['tmp_name']) . ' -auto-orient ' . escapeshellarg($upload));

Doesn't drop the exif, I've tested gm convert on test pictures and it was all still there. So I'm of the opinion, and I've actually tested and verified it, that with the following config:

$config['thumb_method'] = 'gm+gifsicle';
$config['strip_exif'] = true;
$config['convert_auto_orient'] = true;
$config['use_exiftool'] = true; 
$config['redraw_image'] = false;  // default
$config['convert_manual_orient'] = false;  // default

A JPEG with Exif orientation metadata could be posted on an imageboard with fully surviving Exif, while one would normally expect that with $config['strip_exif'] = true; no Exif data would survive.

I don't understand the significance of

$config['use_exiftool'] = true; 

If that's set to false, the orientation logic doesn't work at all due to the check at line L1055 weirdly enough.

Other minor issues with this code are:

• it's checking file extension string instead of file type, in multiple places, maybe not worth improving but 10 lines above it checks in the right way already so might as well
• it's checking file extension twice, at line 1055 the check is redundant
• the orientation conversion command uses $upload and $file['tmp_name']) as arguments, but these are identical values at this point

[Zankaria]

10 years of commits lead to this kind of cruft Thanks for the heads up

[crazy4cars69]

Bet there are more shit like this in vichan code

[papereth]

BTW just in case you haven't noticed it, I have opened a PR with a fix, I've tested this code and noticed nothing wrong so far. https://github.com/vichan-devel/vichan/pull/736