Classic bug: Unterminated team-creation fields

Bug ID	GE-BUG-039
Affected versions	Classic 3.2e and published classic source
Status	Verified in source; corrected in ge-next
Fixed in	ge-next 0.3
Primary source	Team creation in cmd_team(); fixed-size strings in TEAM

Summary

Classic GE does not explicitly null-terminate the team name, member password, or founder secret copied while creating a team. Input that fills the permitted field width can therefore leave each temporary string unterminated.

This is especially significant for passwords because classic's own help uses ten-character passwords in its TEAM START syntax. Ten characters exactly fill the copied portion of an 11-byte field, but the code never writes the required null byte to the final element.

The unterminated temporary values are used for string comparison and displayed in the team-creation confirmation. String functions can read into adjacent members of the temporary TEAM structure and, for the last string, beyond the structure member into unrelated stack data.

The TEAM string fields

Classic declares these fields in TEAM:

char teamname[31];
char password[11];
char secret[11];

Their intended maximum contents are therefore:

Field	Character capacity	Terminator element
Team name	30	teamname[30]
Member password	10	password[10]
Founder secret	10	secret[10]

The extra element in each array is needed for the terminating null byte and is not another usable character.

Faulty creation path

The documented command is:

TEAM START nnnnn ssssssssss pppppppppp <team name>

The founder secret and member password must be saved before rstrin() reconstructs the remainder of the input as the possibly multiword team name. Classic copies all three values into an uninitialized automatic TEAM tmp structure:

strncpy(tmp.secret, margv[3], 10);
strncpy(tmp.password, margv[4], 10);

rstrin();
strncpy(tmp.teamname, margv[5], 30);
tmp.teamcount = 1;

No statement writes zero to tmp.secret[10], tmp.password[10], or tmp.teamname[30].

strncpy() pads its destination with zeros when the source is shorter than the requested count. Short values therefore happen to be safely terminated. When the source contains at least the requested number of characters, however, strncpy() copies exactly that many bytes and does not append a terminator.

The defect is therefore conditional on field length:

• secrets and passwords of one through nine characters are terminated by padding;
• secrets and passwords of ten or more characters are not terminated in tmp;
• team names shorter than 30 characters are terminated by padding; and
• team names of 30 or more characters are not terminated in tmp.

Longer input is truncated to the listed capacity, but truncation does not make the resulting field a valid C string without the final null byte.

Reads beyond the intended strings

The routine uses tmp.teamname in sameas() while checking whether another team already has the name. A full 30-character name can make that comparison continue into padding or the following numeric members of tmp. The result depends on whatever bytes happen to follow the array on that invocation. Duplicate detection can fail, or a valid new name can compare against unintended suffix bytes.

After creating the team, classic passes all three temporary values to a message as %s strings:

prfmsg(TEAMCRT, tmp.teamname, gechrbuf, tmp.password, tmp.secret);

An unterminated member password can continue through its unused final byte and into the founder secret. An unterminated secret can continue into the following structure member and then unrelated stack data until some later zero byte is encountered. The confirmation can consequently display garbage suffixes, combine fields, expose incidental memory, or fail unpredictably.

These are out-of-bounds reads. Their exact visible result is not guaranteed by C and can vary with compiler, build, and the previous contents of the stack.

The second set of incomplete copies

Classic then copies the temporary fields into the global team table without terminating those destinations either:

strncpy(teamtab[next].teamname, tmp.teamname, 30);
strncpy(teamtab[next].password, tmp.password, 10);
strncpy(teamtab[next].secret, tmp.secret, 10);

In a newly started game, teamtab has static storage and begins zero-filled. Classic also allocates new teams only from entries whose teamcode is zero; deleted entries are marked with -1 rather than reused. The untouched final bytes in a fresh slot are therefore normally zero, which often makes the persistent copies appear valid despite the missing code.

That is an incidental property of the surrounding implementation, not a guarantee supplied by these copies. It does not protect the earlier uses of tmp, and it becomes unsafe if a slot contains stale data, is reused, or is populated under different initialization behavior.

An unterminated value that reaches update_team_tab() is especially dangerous because the text database uses %s conversions with minimum field widths. A width such as %-10s pads short strings but does not limit a longer or unterminated one. Extra bytes can expand a field, move the pipe delimiters, and produce a record that the loader later rejects. The team may then be malformed or disappear after the next reload.

Example

Consider the fully documented ten-character inputs:

Founder secret: 1234567890
Member password: abcdefghij

Classic copies ten characters into each 11-byte array. If the uninitialized final byte of tmp.password contains X, its apparent string begins as:

abcdefghijX1234567890...

The first ten characters are the password, X is unrelated stack data in password[10], and the following characters belong to the adjacent secret. The team-creation message and any other unbounded string use cannot know where the password was intended to end.

If that byte happens to be zero, output looks correct. This dependence on an uninitialized byte explains why the defect may appear intermittent.

Correction in ge-next 0.3

ge-next 0.3 explicitly terminates every fixed-width copy at both stages:

strncpy(tmp.secret, margv[3], 10);
strncpy(tmp.password, margv[4], 10);
tmp.secret[10] = 0;
tmp.password[10] = 0;

rstrin();
strncpy(tmp.teamname, margv[5], 30);
tmp.teamname[30] = 0;

The final team-table assignment receives the same protection:

strncpy(teamtab[next].teamname, tmp.teamname, 30);
teamtab[next].teamname[30] = 0;
strncpy(teamtab[next].password, tmp.password, 10);
teamtab[next].password[10] = 0;
strncpy(teamtab[next].secret, tmp.secret, 10);
teamtab[next].secret[10] = 0;

This preserves the documented 30-character name and ten-character password limits while making each value a valid C string regardless of prior stack or table contents. ge-next32 uses the same corrected creation path.

Home · Play · Reference · Versions & history · Sysop · Development · Classic bugs