1533527 - malloc and calloc silently truncate int64_t and uint64_t on 32bit

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement, P1)

Description

[Kelsey Gilbert [:jgilbert]]

This is just terrible design on the part of malloc and calloc.

I'm worried that even enabling truncation warnings will only find this on 32bit builds.

Fortunately, there's a c++ template construct we can use here.

Comment 1

[Kelsey Gilbert [:jgilbert]]

// Prevent implicit conversions into calloc and malloc.

template<typename RequireSizeT1, typename RequireSizeT2>
void* calloc(RequireSizeT1 n, RequireSizeT2 size) = delete;

template<>
void* calloc<size_t, size_t>(size_t n, size_t size) { return ::calloc(n, size); }

template<typename RequireSizeT>
void* malloc(RequireSizeT size) = delete;

template<>
void* malloc<size_t>(size_t size) { return ::malloc(size); }

// uint32_t is also always ok:
template<>
void* calloc<size_t, uint32_t>(size_t n, uint32_t size) { return ::calloc(n, size); }
template<>
void* calloc<uint32_t, size_t>(uint32_t n, size_t size) { return ::calloc(n, size); }
template<>
void* calloc<uint32_t, uint32_t>(uint32_t n, uint32_t size) { return ::calloc(n, size); }
template<>
void* malloc<uint32_t>(uint32_t size) { return ::malloc(size); }

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1533527 - Forbid non-size_t calls to malloc and calloc.

Comment 3

[Kelsey Gilbert [:jgilbert]]

:waldo pointed out that typedefs are undetectable to templates, though this did find some bugs on my x64 build, probably due to unsigned int vs unsigned long.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9049394 [details]
Bug 1533527 - Forbid non-size_t calls to malloc and calloc.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: None of the fixed code seems to be vulnerable, but this is something we should spread to the rest of the tree.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: trivial
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

As a sec-want, this doesn't need sec-approval. We should put this on trunk but it is too late to backport it to 66, which goes to RC next week.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Roger that.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/5f7ea2187fa661b0d3822c2e82a5601d89a50769

Backed out for build bustages:

https://hg.mozilla.org/integration/autoland/rev/f0eaf5676456b5bc7c8d8e642441cbb9b8d9788f

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=retry%2Cusercancel%2Ctestfailed%2Cbusted%2Cexception&group_state=expanded&searchStr=windows%2Cpgo&revision=5f7ea2187fa661b0d3822c2e82a5601d89a50769
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=232958174&repo=autoland

[task 2019-03-10T01:50:59.672Z] 01:50:59 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/gfx/Rect.h:14:
[task 2019-03-10T01:50:59.672Z] 01:50:59 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/gfx/Tools.h:128:41: error: call to deleted function 'calloc'
[task 2019-03-10T01:50:59.673Z] 01:50:59 INFO - mStorage = static_cast<uint8_t >(calloc((size_t)1, storageByteCount.value()));
[task 2019-03-10T01:50:59.673Z] 01:50:59 INFO - ^~~~~~
[task 2019-03-10T01:50:59.674Z] 01:50:59 INFO - /builds/worker/workspace/build/src/dom/canvas/WebGLTypes.h:22:7: note: candidate function [with RequireSizeT1 = unsigned long, RequireSizeT2 = int] has been explicitly deleted
[task 2019-03-10T01:50:59.674Z] 01:50:59 INFO - void calloc(RequireSizeT1 n, RequireSizeT2 size) = delete;
[task 2019-03-10T01:50:59.674Z] 01:50:59 INFO - ^
[task 2019-03-10T01:50:59.674Z] 01:50:59 INFO - /usr/include/malloc.h:54:14: note: candidate function
[task 2019-03-10T01:50:59.674Z] 01:50:59 INFO - extern void *calloc __MALLOC_P ((size_t __nmemb, size_t __size))

Comment 8

[Kelsey Gilbert [:jgilbert]]

I'll nurse this through this week.

Fortunately, this doesn't change functionality, so it can go in during soft-freeze.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5f8c7b771725361f054b02a4a641820482e1f1
https://hg.mozilla.org/mozilla-central/rev/bd5f8c7b7717

Comment 10

[Kelsey Gilbert [:jgilbert]]

Nothing found here seemed to turn up any sec issues, so fix-optional for 67.

I would de-sec this bug, but I don't really want to do that until we expand this approach to the whole tree.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This can ride the trains if it didn't turn up any sec issues.