Skip to content

fix an off by one error in token get #4105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Aug 24, 2024
Merged

fix an off by one error in token get #4105

merged 5 commits into from
Aug 24, 2024

Conversation

jcupitt
Copy link
Member

@jcupitt jcupitt commented Aug 22, 2024

vips__token_get() could read a byte beyond the end of strings containing unterminated quotes.

See #4104

I'll expose this function in pyvips as well so we can add some tests, it's clearly tricker to get right than it looks.

@jcupitt jcupitt changed the base branch from master to 8.15 August 22, 2024 11:24
kleisauke
kleisauke approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kleisauke
Copy link
Member

It looks like the issue isn't fully resolved yet. I encountered a similar issue while fuzzing on this branch locally.

Details 
$ vips copy test/test-suite/images/sample.png x.png[Q=75]\'x\'
=================================================================
==102648==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x50200008f57f at pc 0x7f0d2bc417fa bp 0x7ffe7fb7d220 sp 0x7ffe7fb7d218
READ of size 1 at 0x50200008f57f thread T0
    #0 0x7f0d2bc417f9 in vips__token_get /home/kleisauke/libvips/build/../libvips/iofuncs/util.c:1303:8
    #1 0x7f0d2bc3dfa8 in vips__find_rightmost_brackets /home/kleisauke/libvips/build/../libvips/iofuncs/util.c:1482:9
    #2 0x7f0d2bc3dcf1 in vips_filename_suffix_match /home/kleisauke/libvips/build/../libvips/iofuncs/util.c:498:20
    #3 0x7f0d2b3d4961 in vips_foreign_find_save_target_sub /home/kleisauke/libvips/build/../libvips/foreign/foreign.c:2128:3
    #4 0x7f0d2bc3bed6 in vips_slist_map2 /home/kleisauke/libvips/build/../libvips/iofuncs/util.c:101:33
    #5 0x7f0d2b3d469a in vips_foreign_map /home/kleisauke/libvips/build/../libvips/foreign/foreign.c:489:11
    #6 0x7f0d2b3d469a in vips_foreign_find_save_target /home/kleisauke/libvips/build/../libvips/foreign/foreign.c:2154:46
    #7 0x7f0d2bb98ae8 in vips_image_write_to_file /home/kleisauke/libvips/build/../libvips/iofuncs/image.c:2692:19
    #8 0x7f0d2bb83b99 in vips_object_get_argument_to_string /home/kleisauke/libvips/build/../libvips/iofuncs/object.c:2243:8
    #9 0x7f0d2bbdc884 in vips_call_argv_output /home/kleisauke/libvips/build/../libvips/iofuncs/operation.c:1420:8
    #10 0x7f0d2bb76c1c in vips_argument_map /home/kleisauke/libvips/build/../libvips/iofuncs/object.c:612:17
    #11 0x7f0d2bbdbefb in vips_call_argv /home/kleisauke/libvips/build/../libvips/iofuncs/operation.c:1487:6
    #12 0x5067f5 in main /home/kleisauke/libvips/build/../tools/vips.c:884:7
    #13 0x7f0d2a839087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: 77c77fee058b19c6f001cf2cb0371ce3b8341211)
    #14 0x7f0d2a83914a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: 77c77fee058b19c6f001cf2cb0371ce3b8341211)
    #15 0x42a6e4 in _start (/usr/bin/vips+0x42a6e4) (BuildId: b3e63d689b6f578d2d6a7b733a3d2d4cc7ddf057)

0x50200008f57f is located 0 bytes after 15-byte region [0x50200008f570,0x50200008f57f)
allocated by thread T0 here:
    #0 0x4c5533 in malloc (/usr/bin/vips+0x4c5533) (BuildId: b3e63d689b6f578d2d6a7b733a3d2d4cc7ddf057)
    #1 0x7f0d2c574879 in g_malloc (/lib64/libglib-2.0.so.0+0x63879) (BuildId: 36b60dbd02e796145a982d0151ce37202ec05649)
    #2 0x7f0d2c553e0e in g_path_get_basename (/lib64/libglib-2.0.so.0+0x42e0e) (BuildId: 36b60dbd02e796145a982d0151ce37202ec05649)
    #3 0x7f0d2bc3dce5 in vips_filename_suffix_match /home/kleisauke/libvips/build/../libvips/iofuncs/util.c:494:13
    #4 0x7f0d2b3d4961 in vips_foreign_find_save_target_sub /home/kleisauke/libvips/build/../libvips/foreign/foreign.c:2128:3
    #5 0x7f0d2bc3bed6 in vips_slist_map2 /home/kleisauke/libvips/build/../libvips/iofuncs/util.c:101:33
    #6 0x7f0d2b3d469a in vips_foreign_map /home/kleisauke/libvips/build/../libvips/foreign/foreign.c:489:11
    #7 0x7f0d2b3d469a in vips_foreign_find_save_target /home/kleisauke/libvips/build/../libvips/foreign/foreign.c:2154:46
    #8 0x7f0d2bb98ae8 in vips_image_write_to_file /home/kleisauke/libvips/build/../libvips/iofuncs/image.c:2692:19
    #9 0x7f0d2bb83b99 in vips_object_get_argument_to_string /home/kleisauke/libvips/build/../libvips/iofuncs/object.c:2243:8
    #10 0x7f0d2bbdc884 in vips_call_argv_output /home/kleisauke/libvips/build/../libvips/iofuncs/operation.c:1420:8
    #11 0x7f0d2bb76c1c in vips_argument_map /home/kleisauke/libvips/build/../libvips/iofuncs/object.c:612:17
    #12 0x7f0d2bbdbefb in vips_call_argv /home/kleisauke/libvips/build/../libvips/iofuncs/operation.c:1487:6
    #13 0x5067f5 in main /home/kleisauke/libvips/build/../tools/vips.c:884:7
    #14 0x7f0d2a839087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: 77c77fee058b19c6f001cf2cb0371ce3b8341211)
    #15 0x7f0d2a83914a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: 77c77fee058b19c6f001cf2cb0371ce3b8341211)
    #16 0x42a6e4 in _start (/usr/bin/vips+0x42a6e4) (BuildId: b3e63d689b6f578d2d6a7b733a3d2d4cc7ddf057)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/kleisauke/libvips/build/../libvips/iofuncs/util.c:1303:8 in vips__token_get
Shadow bytes around the buggy address:
  0x50200008f280: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x50200008f300: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x50200008f380: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x50200008f400: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x50200008f480: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
=>0x50200008f500: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00[07]
  0x50200008f580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50200008f600: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50200008f680: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50200008f700: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x50200008f780: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==102648==ABORTING
Aborted (core dumped)

@jcupitt
Copy link
Member Author

jcupitt commented Aug 23, 2024

Yes, I'm adding some tests and revising the code again. There will be another version sigh.

@dloebl dloebl mentioned this pull request Aug 23, 2024
Merged
@jcupitt
Copy link
Member Author

jcupitt commented Aug 24, 2024

I added tests via a tiny C prog and some shell script, rather than via pyvips.

It means we have a new item in the exposed API, which is unfortunate for a stable release :( Perhaps the tests should be moved to master, and 8.15 should only have the fixed-up _token_get()?

@jcupitt
Copy link
Member Author

jcupitt commented Aug 24, 2024

... I moved the tests to master in another PR to avoid changing the stable ABI.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 24, 2024

Tests are now here #4113

kleisauke
kleisauke approved these changes Aug 24, 2024
View reviewed changes
Copy link
Member

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that this resolves the issue. Additionally, after running for two hours, the fuzzers did not identify any new issues related to this function. 👍

@jcupitt
Copy link
Member Author

jcupitt commented Aug 24, 2024

Great! Thank you for testing it, Kleis.

@jcupitt jcupitt merged commit 99b3d18 into 8.15 Aug 24, 2024
10 checks passed
@jcupitt jcupitt deleted the revise-token-get branch August 24, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleisauke kleisauke kleisauke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jcupitt @kleisauke