GNOME Bugzilla – Bug 774804
Use after free in ephy_sync_crypto_compute_hawk_header
Last modified: 2016-11-21 19:49:36 UTC
==27654== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) ==27599== Invalid read of size 8 ==27599== at 0x15268C3C: soup_uri_get_query (soup-uri.c:1155) ==27599== by 0x50BD91C: ephy_sync_crypto_compute_hawk_header (ephy-sync-crypto.c:714) ==27599== by 0x50BEF75: ephy_sync_service_fxa_hawk_post_async (ephy-sync-service.c:187) ==27599== by 0x50C0079: ephy_sync_service_obtain_signed_certificate (ephy-sync-service.c:534) ==27599== by 0x50C016A: ephy_sync_service_issue_storage_request (ephy-sync-service.c:565) ==27599== by 0x50C185A: ephy_sync_service_send_storage_message (ephy-sync-service.c:1031) ==27599== by 0x50C2CD4: ephy_sync_service_sync_bookmarks (ephy-sync-service.c:1475) ==27599== by 0x50C2D0F: do_periodical_sync (ephy-sync-service.c:1489) ==27599== by 0x50C2D71: ephy_sync_service_start_periodical_sync (ephy-sync-service.c:1504) ==27599== by 0x50ABF98: ephy_shell_startup (ephy-shell.c:326) ==27599== by 0x158C99DE: g_cclosure_marshal_VOID__VOID (gmarshal.c:875) ==27599== by 0x158C701B: g_type_class_meta_marshal (gclosure.c:997) ==27599== Address 0x28d0b7a0 is 48 bytes inside a block of size 64 free'd ==27599== at 0x4C2CD5A: free (vg_replace_malloc.c:530) ==27599== by 0x18EDAA45: g_free (gmem.c:189) ==27599== by 0x18EF4C73: g_slice_free1 (gslice.c:1136) ==27599== by 0x15268039: soup_uri_free (soup-uri.c:708) ==27599== by 0x50BD8EA: ephy_sync_crypto_compute_hawk_header (ephy-sync-crypto.c:709) ==27599== by 0x50BEF75: ephy_sync_service_fxa_hawk_post_async (ephy-sync-service.c:187) ==27599== by 0x50C0079: ephy_sync_service_obtain_signed_certificate (ephy-sync-service.c:534) ==27599== by 0x50C016A: ephy_sync_service_issue_storage_request (ephy-sync-service.c:565) ==27599== by 0x50C185A: ephy_sync_service_send_storage_message (ephy-sync-service.c:1031) ==27599== by 0x50C2CD4: ephy_sync_service_sync_bookmarks (ephy-sync-service.c:1475) ==27599== by 0x50C2D0F: do_periodical_sync (ephy-sync-service.c:1489) ==27599== by 0x50C2D71: ephy_sync_service_start_periodical_sync (ephy-sync-service.c:1504) ==27599== Block was alloc'd at ==27599== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) ==27599== by 0x18EDA8F6: g_malloc (gmem.c:94) ==27599== by 0x18EF4A38: g_slice_alloc (gslice.c:1025) ==27599== by 0x18EF4A77: g_slice_alloc0 (gslice.c:1051) ==27599== by 0x15266950: soup_uri_new_with_base (soup-uri.c:283) ==27599== by 0x152676C7: soup_uri_new (soup-uri.c:519) ==27599== by 0x50BD520: ephy_sync_crypto_compute_hawk_header (ephy-sync-crypto.c:633) ==27599== by 0x50BEF75: ephy_sync_service_fxa_hawk_post_async (ephy-sync-service.c:187) ==27599== by 0x50C0079: ephy_sync_service_obtain_signed_certificate (ephy-sync-service.c:534) ==27599== by 0x50C016A: ephy_sync_service_issue_storage_request (ephy-sync-service.c:565) ==27599== by 0x50C185A: ephy_sync_service_send_storage_message (ephy-sync-service.c:1031) ==27599== by 0x50C2CD4: ephy_sync_service_sync_bookmarks (ephy-sync-service.c:1475)
So the problem is: // Free the URI soup_uri_free (uri); if (options == NULL || options->nonce == NULL) g_free (nonce); // Use after free if (soup_uri_get_query (uri) != NULL) g_free (resource); I don't like how resource is sometimes owned and must be freed, and sometimes unowned. It's simpler to just use g_strdup and always own the memory. Ditto with nounce.
Also it looks like hash is leaked
Created attachment 340463 [details] [review] sync-crypto: Clarify memory ownership We were sometimes leaking hash; this fixes that. Also, we had a use-after-free here using the SoupURI after it had already been freed. Gabriel, want to review this?
Review of attachment 340463 [details] [review]: This looks good, the code is much cleaner now without those conditional frees. The patch is good to be accepted.
Attachment 340463 [details] pushed as 15c52b1 - sync-crypto: Clarify memory ownership