Hi,
On Tue, Oct 27, 2015 at 05:56:41PM +0100, Mildis wrote:
> Hi,
>
> I???ve reworked the patch.
> No warnings whatsoever, wether using GCC or LLVM.
> checkpatch is OK, no warnings either.
> Buffers are secured and all loops exit naturally or with an index I???ve
> checked against off-by-one.
OK, thanks.
> I???ve made a custom loop to remove the brackets : I found it easier than
> to do arithmetic around numerous strlen calls.
> Also, I???ve declared the address buffer of with a size of 48 as it looks
> that INET6_ADDRSTRLEN is sometime define???d as 48, sometimes as 46.
OK, comments below :
diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..04917e4 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -620,9 +620,53 @@ const char *invalid_domainchar(const char *name) {
struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa,
int resolve)
{
struct hostent *he;
+ /* max IPv6 length, including brackets and terminating NULL */
+ char tmpip[48];
+ /* point to str by default, if FQDN processing is required */
+ const char *tmpipptr;
+
+ /* check IPv6 with square brackets */
+ if (str[0] == '[') {
+ size_t iplength = strlen(str);
+
+ if (iplength < 4) {
+ /* minimal size is 4 when using brackets "[::]" */
+ goto fail;
+ }
+ else if (iplength >= sizeof(tmpip)) {
+ /* IPv6 literal can not be larger than tmpip */
+ goto fail;
+ }
+ else {
+ if (str[iplength - 1] != ']') {
+ /* if address started with bracket, it should
end with bracket */
+ goto fail;
+ }
+ else {
+ int srcidx, dstidx;
+
+ /* copy and strip the brackets */
+ for (srcidx = 1, dstidx = 0; dstidx < 47;
srcidx++, dstidx++) {
+ if (str[srcidx] == ']')
+ break;
+ tmpip[dstidx] = str[srcidx];
+ }
Here this loop is confusing, useless and apparently wrong since it stops
on the first closing bracket while you tested for the presence of the
last one, so nothing guarantees it's the same. You know from the "if"
above that str[iplength - 1] == ']', so you know the length of the address :
it's iplength - 2, so what you need instead of this loop is simply :
memcpy(tmpip, str + 1, iplength - 2);
tmpip[iplength - 2] = 0;
+
+ /* secure buffer */
+ for (; dstidx <= 47; dstidx++)
+ tmpip[dstidx] = '\0';
This is useless, you just needed tmpip[dstidx] = 0. Also please note
that every time a safety measure is overdesigned, it indicates a lack
of trust on the rest of the code. The comment "secure buffer" clearly
indicates this. There's nothing to secure here, just a string to
terminate.
+ /* switch to tmpip as we are working on a
literal bracketed IPv6 */
+ tmpipptr = tmpip;
You can avoid changing all the pointers below from str to tmpipptr by
simply doing "str = tmpip" here.
The changes to str2ip2() look OK though.
Regards,
Willy
