Re: [PATCH 1/3] Added net_bootp6 command

[Michael Chang]

On Sun, Apr 19, 2015 at 11:15:21AM +0300, Andrei Borzenkov wrote:
> В Fri, 17 Apr 2015 13:04:27 +0800
> Michael Chang <address@hidden> пишет:
> 
> > > 
> > > > +static const struct grub_dhcpv6_option*
> > > > +find_dhcpv6_option (const struct grub_net_dhcpv6_packet *packet,
> > > > +                   grub_uint16_t option)
> > > > +{
> > > > +  grub_uint16_t code, len;
> > > > +  const struct grub_dhcpv6_option *popt;
> > > > +
> > > > +  popt = (const struct grub_dhcpv6_option *)packet->dhcp_options;
> > > > +  code = grub_be_to_cpu16 (popt->code);
> > > > +  len = grub_be_to_cpu16 (popt->len);
> > > > +
> > > > +  while (0 != code && option != code)
> > > 
> > > This probably needs upper boundary check in case we are dealing with
> > > corrupted packets.
> > 
> > 
> > Do you mean checking the option length can't exceed netbuff length?
> > 
> 
> Yes (and in all other cases below as well).

OK.

> 
> > The ( 0!= code ) did certain kind of upper boundary check for me, but
> > I know you may disagree that. :)
> > 
> > Btw, in grub-core/net/ip.c::handle_dgram() has checksum for the receiving
> > packets, so the corrputed packets wouldn't sneak in easily.
> > 
> 
> Correct checksum just means content was not altered; it does not mean
> content was correct to start with.

I agree with you that it doesn't mean the content was OK to start with,
the sender might intentionally or unintentionally fill the payload
length wrong that triggers buffer overflow on the client side.

FWIW, the answer is subject to "corrupted packet" that usually means
corruption during the transit and checksum is generally used for
detecting such errors . 

> 
> > > > +
> > > > +  ip_start = ip_end = NULL;
> > > > +  ip_start = bootfile_url + grub_strlen(pr);
> > > > +
> > > > +  if (*ip_start != '[')
> > > > +    ip_start = NULL;
> > > > +  else
> > > > +    ip_end = grub_strchr (++ip_start, ']');
> > > > +
> > > > +  if (!ip_start || !ip_end)
> > > > +    {
> > > > +      grub_error (GRUB_ERR_IO, N_("IPv6-address not in square 
> > > > brackets"));
> > > > +      goto cleanup;
> > > > +    }
> > > > +
> > > 
> > > Can bootfile_url contain a name and not IPv6 address? Or is address
> > > mandatory?
> > 
> > Yes. The string in URL form is mandatory.
> >
> 
> I probably was not clear. Must it always be IPv6 address literal as
> implied by code or can it be e.g. DNS name?

The DNS name could be legit for URL, but current elilo and edk2
explicitly checks for the starting and ending delimiter '[..]' for using
the server address and any name is treated as invalid parameters. The
implementation just followed them.

> 
> > > > +struct grub_net_network_level_interface *
> > > > +grub_net_configure_by_dhcpv6_reply (const char *name,
> > > > +       struct grub_net_card *card,
> > > > +       grub_net_interface_flags_t flags,
> > > > +       const struct grub_net_dhcpv6_packet *v6,
> > > > +       grub_size_t size __attribute__ ((unused)),
> > > > +       int is_def,
> > > > +       char **device, char **path)
> > > > +{
> > > > +  grub_net_network_level_address_t addr;
> > > > +  grub_net_network_level_netaddress_t netaddr;
> > > > +  struct grub_net_network_level_interface *inf;
> > > > +  const grub_uint8_t *your_ip;
> > > > +  char *proto;
> > > > +  char *server_ip;
> > > > +  char *boot_file;
> > > > +  grub_net_network_level_address_t *dns;
> > > > +  grub_uint16_t num_dns;
> > > > +
> > > > +  if (device)
> > > > +    *device = NULL;
> > > > +
> > > > +  if (path)
> > > > +    *path = NULL;
> > > > +
> > > > +  if (v6->message_type != DHCPv6_REPLY)
> > > 
> > > Is it really possible? We come here if packet is DHCPv6_REPLY only.
> > 
> > I'm in case some evil firmware to cache some other packets type rather
> > than reply (or even junks in it ..).
> >
> 
> Yes, but what I mean - this function is only called when we already
> checked for message_type == DHCPv6_REPLY. So the only reason it can be
> different here is physical memory corruption.

It also gets called from grub_efi_net_config_real () which doesn't have
any check for message type.

Thanks,
Michael