COPY may access undefined memory
Closed this issue · 5 comments
The COPY(src, dst) function copies character data from character array src to dst.
At most LEN(dst) characters are copied, and copying stops when a terminating 0X is found. This is the implementation in SYSTEM.Oh.
#define __COPY(s, d, n) {char*_a=(void*)s,*_b=(void*)d; INTEGER _i=0,_t=n-1; \
while(_i<_t&&((_b[_i]=_a[_i])!=0)){_i++;};_b[_i]=0;}
If src is unterminated, and is shorter than dst, then undefined data beyond the end of src will be copied.
For example, if LEN(src)=10, and LEN(dst) is 1000, then 990 random bytes of heap or stack may be accessed.
Its possible, though unlikely, that this could trigger a segfault. For example, if src is at the end of the heap, then the copy could access addresses that are unmapped.
For the call COPY(s, d) the compiler generates __COPY(s, d, LEN(d)).
To be safer, it should probably generate __COPY(s, d, MIN(LEN(s), LEN(d)))
I admit that this is fairly unlikely to cause a problem, so may not be worth it if its difficult to fix.
I have absolutely no objection to making Ofront+ more secure, reliable, and predictable.
The tricky thing about COPY is that COPY can silently truncate a string if there wasn't enough size in destination. And thus distort it.
Copying strings in Component Pascal, with the syntax using the $ sign, is more secure:
str1 := str2$; (* COPY(str2, str1) *)
The copy operation $ in Component Pascal protects the integrity and immutability of a string - it will issue a warning if there is no place to copy.
Btw, I've moved COPY in SYSTEM for Oberon-07 and Component Pascal for exactly that reason.
But for Oberon and Oberon-2, we need to leave this operation, and we can make it safer.
By implementation: the simplest thing that came to my mind was to pass both lengths inside the __COPY macro. And then calculate the minimum length:
#define __COPY(s, d, n, n2) {char*_a=(void*)s,*_b=(void*)d; INTEGER _i=0,_t=n-1;if(_t>=n2)t=n2-1; \
while(_i<_t&&((_b[_i]=_a[_i])!=0)){_i++;};_b[_i]=0;}
I didn't test this macro.
For efficiency, I suggest using a static inline function instead of a macro. Otherwise, n2 will be calculated twice, which is bad if it is a complex expression. Also, twice calculation can have a side effect if it uses a function call that should only be called once.
In C++ there is min macro in stdlib.h, but this function are not defined by the ANSI C standard. And I'm not sure that the same problem with twice calculations is solved here.
After thinking on this task, I still decided not to pass both lengths and write the macro __MIN, since an array length cannot has a complex expression that can be calculated twice. The length value passed inside the macro __COPY can only be a constant or the field "len" of the structure that represents one-dimensional character array.
The implemented commit still has the flaw of using __MIN for both constants.
__COPY(name, m->name, __MIN(20, 20));
This can be corrected by adding some complexity to the code.
Yeah, I did it:
__COPY(name, m->name, 20); // 20 is a minimal of the both known in compile-time lengths
__COPY(name, ent->name, __MIN(256, name__len)); // 256 is known, name__len is not known in compile-time
__COPY(msg, (*res).msg->data, __MIN((*res).msg->len[0], msg__len)); // both lengths are unknown in compile-time
I found another case where there is a dangerous copying of a string with an out-of-memory.
MODULE TestCopy;
VAR str: ARRAY 10 OF CHAR;
BEGIN
COPY("str1235576856384762", str)
END TestCopy.
__MOVE("str1235576856384762", TestCopy_str, 20); // <- length must be 10
__ENDMOD;
Here I've used optimization from CPfront. But in BlackBox, the function COPY is considered obsolette, so it is not available without a special option that few people know how to use.
So on this case Ofornt+ generates __MOVE call instead of __COPY. Here I decided that a memory copying without comparing to zero would be more efficient than copying with comparing to zero.
Simple fix (OPV.stat):
IF l^.typ^.comp IN {Array, DynArr} THEN
IF (r^.typ^.form = String) & (r^.class # Nconst) THEN
StringCopy(l, r, FALSE)
ELSE
OPM.WriteString(MoveFunc);
expr(r, MinPrec); OPM.WriteString(Comma); expr(l, MinPrec); OPM.WriteString(Comma);
IF r^.typ = OPT.stringtyp THEN i := r^.conval^.intval2
ELSE i := r^.typ^.size
END;
OPM.WriteInt(MIN(i, l^.typ^.size)); OPM.Write(CloseParen)
END
ELSE
This fix makes copying safe, but unfortunately does not solve the problem if the string is larger than the destination - a part of the string is copied without a trailing zero. I'm still thinking about this problem. It might even be worth going back with __COPY.
Ofront and voc do not have this problem. But it is present in CPfront.
I have introduced a new type in Ofront+ that is not present in Ofront/voc. It's a system pointer to array without length. We can go deeper into the discussion about unsafety of this type, but I don't encourage it to be used widely. This is only necessary for the convenience of system programming. BlackBox also has this type, and I managed to get used to all its amenities.
This type is denoted as POINTER TO ARRAY [untagged] OF ...
Such pointer to untagged array type is very useful when describing bindings, since it is compatible with string literals and arrays of char, as well as pointers to strings. We can think of this type as a restricted address type, which, while still dangerous, is more strictly limited to pointing to strings.
And I certainly had to think about allowing this type to be used in safe COPY operation, which is so safe that it even agrees to sacrifice the contents of the string and truncate it.
In fact, we could only prohibit this type as a COPY source, thereby reducing its value. But I did something else: I allowed this type to be used as a source, with the length calculation via the built-in __STRLEN operation. This is where the danger lies if we submit to the COPY a random value that does not indicate a string with a trailing zero. But we make this sacrifice only for the sake of the usefulness and applicability of this dangerous system type.
Argument of this type is not allowed as a COPY destination: Ofront+ will generate error 137 "unknown array length". I decided that this way we will keep the COPY more safe. After all, we can only calculate the size of a "string" for the dangerous type by using the __STRLEN operation, which will require pre-clearing the destination buffer, or copying without a known length. But we still have SYSTEM.MOVE for similar operation.
If don't use this dangerous type as source, COPY should be completely safe now.