Bug 97333 - net/isc-dhcp3-server: fix size of OPTION 51 in DHCPOFFER for *64 ARCHS
Summary: net/isc-dhcp3-server: fix size of OPTION 51 in DHCPOFFER for *64 ARCHS
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: Sergey Matveychuk
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-05-16 06:40 UTC by Joerg Pulz
Modified: 2006-05-22 01:20 UTC (History)
1 user (show)

See Also:


Attachments
net_isc-dhcp3-server.diff (2.01 KB, patch)
2006-05-16 06:40 UTC, Joerg Pulz
no flags Details | Diff
patch-server-dhcp.c (1.72 KB, text/plain)
2006-05-22 01:16 UTC, Dan Lukes
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joerg Pulz 2006-05-16 06:40:18 UTC
	Beginning with dhcp-3.0.4 the ISC people have slightly changed the way
	the OPTION 51 (IP address lease time) is handled.
	Unfortunately, they broke it for all FreeBSD *64 ARCHS, as time_t is
	uint64_t (8 bytes) on these, instead of uint32_t (4 bytes) like it is
	for all other ARCHS.
	This results in DHCPOFFERs containing an 8 bytes OPTION 51 field which
	is silently dropped/ignored on, at least, Windows XP clients.

	This patch provides a quick fix to solve this problem by setting the
	TIME macro to uint32_t instead of time_t, which obviously results in
	some compiler warnings, because gmtime() and time() require an uint64_t
	value as argument.
	Anyway, it compiles and is working for three people + me, who have
	tested the patch.

	I contacted the ISC DHCP developers about this problem and they have now
	a ticket for this. They will hopefully provide a real patch soon.

	Special thanks to the people who reported this and have tested the
	patch.

Fix: - apply the patch
Comment 1 Sergey Matveychuk freebsd_committer freebsd_triage 2006-05-16 06:52:35 UTC
Responsible Changed
From-To: freebsd-ports-bugs->sem

Take it.
Comment 2 Sergey Matveychuk freebsd_committer freebsd_triage 2006-05-16 07:21:44 UTC
State Changed
From-To: open->closed

Committed. Thanks!
Comment 3 Dan Lukes 2006-05-21 18:34:34 UTC
	Please note, that the way the problem has been fixed is broken and 
dangerous - especially on *64 architectures.

	You forced the size of time-related variables to 32bit wide redefining 
TIME from time_t to u_int32_t.

	Unfortunately, those variables are sometime used in gmtime(&variable) 
or time(&variable) calls - for example, on several places in server/db.c

	Those functions didn't know you passed the pointer to variable of 
insufficient size to it, so it fill eight bytes - because it's expected 
size of the variable memory. It overwrites the 4 byte of memory after 
end of 4-byte declared variables. It may overwrite another variable or 
damage the stack. It may cause deterministic or non-deterministic 
malfunctions of server or may cause the (possibly random) abend of the 
process.

	In advance, rewriting of random parts of memory or stack may have 
security-related consequences.

	If the size of OPTION 51 is defined to be 4 byte but isc-dhcp sends 
another size, then bug seems to be in the routine assembling the 
response packet. It is the correct place for patch.

	As I didn't analyzed the problem itself, I can't supply the correct 
patch. I searched why the compiler (on i386 architecture) emit several 
warnings about passing arguments of incorrect type to a function only.

	Please, don't ignore warnings in the future - they may point to true 
errors (as it is in this case). IMHO, your patch should never be 
committed as too dangerous ...


	Sincerely

					Dan Lukes

	

-- 
Dan Lukes                                   SISAL MFF UK
AKA: dan@obluda.cz, dan@freebsd.cz,dan@kolej.mff.cuni.cz
Comment 4 Sergey Matveychuk freebsd_committer freebsd_triage 2006-05-21 18:54:06 UTC
Dan Lukes wrote:
 >     Those functions didn't know you passed the pointer to variable of
> insufficient size to it, so it fill eight bytes - because it's expected
> size of the variable memory. It overwrites the 4 byte of memory after
> end of 4-byte declared variables. It may overwrite another variable or
> damage the stack. It may cause deterministic or non-deterministic
> malfunctions of server or may cause the (possibly random) abend of the
> process.

Sounds reasonable. I'll mark the port as FORIDDEN on amd64 platform
while we have no appropriate patch.

-- 
Dixi.
Sem.
Comment 5 Dan Lukes 2006-05-22 01:16:21 UTC
	OK, but the previous fix should be reverted. It's bad on *64 and has no 
reason for *i386.

	At least, somebody must remember that the fix must be removed before 
the port will be un-forbidden.

	Well, it's up to appropriate responsible person decision.

	Please note, the 51 is not only problematic options. The same problems 
has option 58 and 59.

	I attached the patch that correct the response packet assembly routine 
for all of them. Or, it try it at least.

	The patch is based on code analysis only. It passed the compilation on 
i386 machine. I never tried the patched code server to run on i386 nor 
*64 architecture by self. Someone else should review and test it.

	If it be classified OK, the the tester shall send it to ISC people and 
create new PR for FreeBSD (unless Sergey reopen this one to feedback state).
	
	Even if it works, there may be other places with sizeof 
(time_t)!=sizeof(u_int32_t) which are not covered by attached patch.

	Hope it help.

					Dan


-- 
Dan Lukes                                   SISAL MFF UK
AKA: dan@obluda.cz, dan@freebsd.cz,dan@kolej.mff.cuni.cz