Opened 3 years ago

Closed 3 years ago

#1216 closed defect (fixed)

string-ref specialization elides range check

Reported by: syn Owned by:
Priority: critical Milestone: 4.11.0
Component: scrutinizer Version: 4.10.x
Keywords: Cc:
Estimated difficulty:

Description

The types.db entry for string-ref currently looks like this:

(string-ref (#(procedure #:clean #:enforce) string-ref (string fixnum) char)
	    ((string fixnum) (##core#inline "C_subchar" #(1) #(2))))

However, unlike C_subchar, the unspecialized version of string-ref (which is really C_i_string_ref) doesn't just check its argument type but also whether the fixnum argument is within range of the string argument. Thus, the specialization may result in buffer overruns, posing a potential vulnerability.

Change History (5)

comment:1 Changed 3 years ago by syn

I would guess that similar issues exist for other specializations, too.

comment:2 Changed 3 years ago by sjamaan

  • Priority changed from major to critical

Changing it to C_i_string_ref would still be beneficial due to inlining. Note that there are also two rewrites in c-platform.scm; an unsafe one that uses C_subchar and a safe one that uses C_i_string_ref.

I think the real problem here is that there are two kinds of unsafe: the kind that doesn't check its argument types (which will result in crashes when passed the wrong type; but in the scrutinizer that means it wouldn't be unsafe because the types are checked elsewhere) and the kind that doesn't check anything. The latter should never be used in specializations, because they will result in unsafe code, which in turn may result in security nightmares.

I'm raising this ticket to "critical" because there may be some true landmines waiting to go off in Scheme code that's otherwise safe. Rewrites like this change the semantics of the code in such a way that you can't reason about its safety anymore.

Last edited 3 years ago by sjamaan (previous) (diff)

comment:3 Changed 3 years ago by sjamaan

  • Milestone changed from someday to 4.11.0

comment:4 Changed 3 years ago by sjamaan

About those rewrites: This may be another instance of #1122

comment:5 Changed 3 years ago by sjamaan

  • Resolution set to fixed
  • Status changed from new to closed

Fixed by 3a15b29 and 55e9d1e

Note: See TracTickets for help on using tickets.