2003 March 2 Added additional bullet-proofing to the audio packet decompression performed by playbuffer() in speaker.c. Before entering the decompression code, a check is made for more than one supposedly mutually-exclusive compression mode being set. If that passes, for each kind of decompression, the compressed length is checked against the maximum ever expected (which is guaranteed not to overflow the decode buffer) and, where appropriate, that the compressed data is an even number of compression frames. Failing any of these tests prints a message on standard error which identifies the sending host and the nature of the error and discards the packet. This corrects the ADPCM buffer overflow exploit reported by the Hackademy Audit (http://www.thehackademy.net/audit.php). [ The buffer length is 512*2 bytes, however the received packet can be longer, and will be expanded by twice its size into the buffer by the ADPCM decompression routine ] 2003 March 3 As discovered in the Hackademy Audit, when PGP key exchange was used, speaker.c created a temporary file in /tmp which could potentially be exploited by a race condition via a symbolic link to overwrite another file or, if the user's umask() was set insecurely, be read by a third party on the local system, compromising the key. I added secure temporary file creation routines in the new file tempfile.c, based on a modified version of the code given in: http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html which adds the ability to leave the file to be unlinked later by the caller and to retrieve the full path name as generated from the environment variables TMPDIR or TMP. I modified the code which invokes PGP to exchange the session key to use this new create_tempfile_in_tempdir(), keeping the file open until it's been processed by PGP, preventing a "jump-in" race condition. Since the umask() of the file is forced to 077, the contents of the file should be secure against anybody other than the super-user. The same temporary file risk existed for the files in which face data were saved for display with the external viewer program. I modified the face display code in speaker.c to use create_tempfile_in_tempdir(), keep the face file open until the viewer terminates, then delete it. This resolves the Hackademy Audit issues of possible buffer overflow composing a face file name from a host name, and the risk of a host name resolving to a string which might permit directory traversal. As no exogenous input contributes to the face file name, these issues can be closed. Hackademy reported a potential vulnerability due to the validation of packet length versus contents being bypassed when the faceRequest bit was set in the packet header. That particular test was due to my having unwisely opted to re-use the packet payload length field for the address of the data being requested in a face image file, which means it cannot be tested against the received packet length as for other packets. Fortunately, these faceRequest packets are all precisely the same length as they bear no payload, and have a unique value in their flags (a.k.a. "compression") field. I added a test which verifies these, which should stop any attempt to spoof downstream code long before it gets there. Packets which fail this validation are reported to standard error and dropped. [ The packet length sanity check can be bypassed using a compression flag with the 0x00000001 bit set ( == faceRequest == Comp2X), but no fFaceData bit. That means the buffer_len can be fake, which allows easier exploitation of other issues. Because arbitrary length can be given, it is also possible to trigger various stack overflows (Denial of Service) and a (probably) remotely exploitable integer overflow. ] The Hackademy Audit revealed a potential buffer overflow exploit if a VAT protocol client sent a packet with a user name longer than the 256 byte stack buffer used to edit it for display when showhosts is set. I rewrote the code to truncate the user name in the packet at 255 characters if it's longer than that. The uname field in the connection is actually 4096 characters, but only a malicious client will attempt to send a VAT packet with an ID longer than 255 characters, so there's no reason to do anything beyond protecting ourselves against such an attack. Note that we don't have to worry about this kind of buffer overflow when parsing items from RTP SDES packets, as their length is constrained to 255 bytes (see copySDESitem() in rtpacket.c). Hackademy claim that the fLoopback facility (which causes Speak Freely packets to be echoed back to the sender) has the potential for abuse since UDP packets can be sent with forged source IP addresses. Well, yes...but here I'm going to quibble. First of all, the fLoopback occurs only after the packet has passed all of the aforementioned consistency checks and been deemed a valid Speak Freely packet. Given that, the only kind of sites you're likely to be able to attack with spoofed loopback packets is other Speak Freely sites, which you can attack far more efficiently by just sending the packets directly as opposed to finding a site to bounce them off with loopback. Sure, you can use this to disguise your identity, but then you might just as well forge the identity in the first place and dispense with the loopback cut-out. The Speak Freely loopback facility cannot be used as a packet multiplier for denial of service requests--it's one packet in, one packet out, so the rate at which loopback packets are sent is limited to the rate at which they can be delivered to the site reflecting them, which cannot be greater than the bandwidth between the attacker and the victim. So...unless anybody finds a flaw in this argument, or produces evidence that the fLoopback facility is, in fact, being abused, I'm going to leave it as-is for the moment. (N.b. It would not be a Big Thing if fLoopback were simply disabled. Almost nobody uses it, as it's much easier to test with the public echo servers which delay audio before sending it back. The fLoopback gimmick dates from 1991, when it was the only practical way to test performance on point to point leased line connections. The same temporary file risk discovered by Hackademy in speaker.c also existed in mike.c's code which invokes PGP to encrypt session keys for transmission to connected parties. I rewrote the code to use the new create_tempfile_in_tempdir() function in in tempfile.c and changed the logic so the newly created temporary file is not closed until PGP is finished with it.