| Message ID | 20230606131733.61216-1-nick.potenski@garmin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix calculation and reading of end-of-file padding | expand |
Hi Nick, On 06.06.23 15:17, 'Nick Potenski' via swupdate wrote: > The stream interface would not read all data from the socket due to two > separate issues. This could cause swupdate to close the socket fd > before a client has written all data, which results in an EPIPE on the > client. > Let's start to explain how this is thought to work. SWUpdate has two interfaces (ctrl and progress), and results about the update should be retrieved via the progress instead of relying on the sending of the data. Anyway, the issue here is generated by standard cpio , that adds padding at the end of the file. Reason for SWUpdate to close the pipe is to avoid that an attacker just appends dummy data at the end of a valid SWU creating a DOS attack if it is not recognized by SWUpdate. That means SWUpdate will read some bytes more (padding generated by cpio), and then it closes definetly independently if more data will be sent. From SWUpdate point of view, it makes no sense to pad. The SWUGenerator project does not use cpio at all, and SWU are terminated with CPIO traile rwithout padding. It is thinkable to do the same in meta-swupdate. > First, stream_interface.c's extract_files() reset its offset variable > before processing each file. This is the way it is thought. offset is valid just for the extraction of each artifact and not valid anymore. > This value is 4-byte aligned This is specified in cpio > but not > 512-byte aligned, so when it was used by cpio-utils.c's > extract_padding() to compute the expected padding, it would produce an > incorrect padding amount. Reading the padding is a sort of best effort. > > Second, cpio-utils.c's extract_padding() could fail to read all padding > in cases where the client uses a smaller chunk size, > especially one less > than the padding size, such as the swupdate-client reference > implementation's 256 byte chunk size. I do not remmember why I set it 256 bytes, probably it was a copy&paste, it is surely too small, this can be changed. > > Signed-off-by: Nick Potenski <nick.potenski@garmin.com> > --- > core/cpio_utils.c | 14 +++++++++++--- > core/stream_interface.c | 1 - > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index bcf4818..6408c8d 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -106,14 +106,22 @@ void extract_padding(int fd, unsigned long *offset) > return; > > padding = (512 - (*offset % 512)) % 512; > - if (padding) { > - TRACE("Expecting %d padding bytes at end-of-file", padding); > + if (!padding) > + return; > + > + TRACE("Expecting %d padding bytes at end-of-file", padding); > + do { > len = read(fd, buf, padding); And probably instead of blocking, we should try in not blocking mode. Data are just dummies. > if (len < 0) { > DEBUG("Failure while reading padding %d: %s", fd, strerror(errno)); > return; > } > - } > + padding -= len; > + } while (len > 0); > + > + if (padding > 0) > + DEBUG("Expected %d more bytes of padding", padding); > + return; I tend to ignore at all, it is not defined that the SWU *must* have a padding. > > return; > } > diff --git a/core/stream_interface.c b/core/stream_interface.c > index 34435fa..8c7a047 100644 > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -229,7 +229,6 @@ static int extract_files(int fd, struct swupdate_cfg *software) > (skip == SKIP_FILE ? "Not required: skipping" : "required")); > > fdout = -1; > - offset = 0; This does not seem correct to me. > > /* > * If images are not streamed directly into the target Regards, Stefano Babic
On Fri, Jun 23, 2023 at 5:26 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi Nick, > > On 06.06.23 15:17, 'Nick Potenski' via swupdate wrote: > > The stream interface would not read all data from the socket due to two > > separate issues. This could cause swupdate to close the socket fd > > before a client has written all data, which results in an EPIPE on the > > client. > > > > Let's start to explain how this is thought to work. SWUpdate has two > interfaces (ctrl and progress), and results about the update should be > retrieved via the progress instead of relying on the sending of the > data. Anyway, the issue here is generated by standard cpio , that adds > padding at the end of the file. > > Reason for SWUpdate to close the pipe is to avoid that an attacker just > appends dummy data at the end of a valid SWU creating a DOS attack if it > is not recognized by SWUpdate. That means SWUpdate will read some bytes > more (padding generated by cpio), and then it closes definetly > independently if more data will be sent. The problem with this approach is that it means that legitimate clients have no way of knowing if the server got the entire update or not; when the EPIPE fails the client is left wondering how much was actually sent and can't differentiate between the server simply decided to close the connection because it thinks padding is unimportant and the server disconnecting because e.g. crashed or rejected the connection, etc. When dealing with sockets, it's really good practice for the server to read everything sent to it until it becomes apparent the connection is going nowhere and it terminates. Could some compromise be accomplished by having the server read padding, but bound it by some upper limit that should never be encountered by well behaved clients (maybe up to 4K)? Beyond that it might actually be a DOS, but won't affect well behaved clients. Also, this is not very good DOS protection; a bad client can just keep re-opening the connection and sending more data on a new connection, or send data really slowly and monopolize the connection. IMHO, the current implementation is just hurting well behaved clients without actually offering much protection. > > From SWUpdate point of view, it makes no sense to pad. The SWUGenerator > project does not use cpio at all, and SWU are terminated with CPIO > traile rwithout padding. It is thinkable to do the same in meta-swupdate. > > > First, stream_interface.c's extract_files() reset its offset variable > > before processing each file. > > This is the way it is thought. offset is valid just for the extraction > of each artifact and not valid anymore. > > > This value is 4-byte aligned > > This is specified in cpio > > > but not > > 512-byte aligned, so when it was used by cpio-utils.c's > > extract_padding() to compute the expected padding, it would produce an > > incorrect padding amount. > > Reading the padding is a sort of best effort. > > > > > Second, cpio-utils.c's extract_padding() could fail to read all padding > > in cases where the client uses a smaller chunk size, > > especially one less > > than the padding size, such as the swupdate-client reference > > implementation's 256 byte chunk size. > > I do not remmember why I set it 256 bytes, probably it was a copy&paste, > it is surely too small, this can be changed. > > > > > Signed-off-by: Nick Potenski <nick.potenski@garmin.com> > > --- > > core/cpio_utils.c | 14 +++++++++++--- > > core/stream_interface.c | 1 - > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > > index bcf4818..6408c8d 100644 > > --- a/core/cpio_utils.c > > +++ b/core/cpio_utils.c > > @@ -106,14 +106,22 @@ void extract_padding(int fd, unsigned long *offset) > > return; > > > > padding = (512 - (*offset % 512)) % 512; > > - if (padding) { > > - TRACE("Expecting %d padding bytes at end-of-file", padding); > > + if (!padding) > > + return; > > + > > + TRACE("Expecting %d padding bytes at end-of-file", padding); > > + do { > > len = read(fd, buf, padding); > > And probably instead of blocking, we should try in not blocking mode. > Data are just dummies. > > > if (len < 0) { > > DEBUG("Failure while reading padding %d: %s", fd, strerror(errno)); > > return; > > } > > - } > > + padding -= len; > > + } while (len > 0); > > + > > + if (padding > 0) > > + DEBUG("Expected %d more bytes of padding", padding); > > + return; > > I tend to ignore at all, it is not defined that the SWU *must* have a > padding. > > > > > return; > > } > > diff --git a/core/stream_interface.c b/core/stream_interface.c > > index 34435fa..8c7a047 100644 > > --- a/core/stream_interface.c > > +++ b/core/stream_interface.c > > @@ -229,7 +229,6 @@ static int extract_files(int fd, struct swupdate_cfg *software) > > (skip == SKIP_FILE ? "Not required: skipping" : "required")); > > > > fdout = -1; > > - offset = 0; > > This does not seem correct to me. > > > > > /* > > * If images are not streamed directly into the target > > Regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== > > -- > You received this message because you are subscribed to the Google Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/6f83276d-30f4-7c96-0560-96e2cae2dcff%40denx.de.
Hi Joshua, Nick, On 26.06.23 16:38, Joshua Watt wrote: > On Fri, Jun 23, 2023 at 5:26 AM Stefano Babic <sbabic@denx.de> wrote: >> >> Hi Nick, >> >> On 06.06.23 15:17, 'Nick Potenski' via swupdate wrote: >>> The stream interface would not read all data from the socket due to two >>> separate issues. This could cause swupdate to close the socket fd >>> before a client has written all data, which results in an EPIPE on the >>> client. >>> >> >> Let's start to explain how this is thought to work. SWUpdate has two >> interfaces (ctrl and progress), and results about the update should be >> retrieved via the progress instead of relying on the sending of the >> data. Anyway, the issue here is generated by standard cpio , that adds >> padding at the end of the file. >> >> Reason for SWUpdate to close the pipe is to avoid that an attacker just >> appends dummy data at the end of a valid SWU creating a DOS attack if it >> is not recognized by SWUpdate. That means SWUpdate will read some bytes >> more (padding generated by cpio), and then it closes definetly >> independently if more data will be sent. > > The problem with this approach is that it means that legitimate > clients have no way of knowing if the server got the entire update or > not; when the EPIPE fails the client is left wondering how much was > actually sent and can't differentiate between the server simply > decided to close the connection because it thinks padding is > unimportant and the server disconnecting because e.g. crashed or > rejected the connection, etc. In fact, decision should be done implementing the progress API, too. But agree this behavior is nasty. > > When dealing with sockets, it's really good practice for the server to > read everything sent to it until it becomes apparent the connection is > going nowhere and it terminates. Well, the SWU contains a termination (the TRAILER string), and SWUpdate can detect if EOF is reached. Any data after the TRAILER is detected is useless. > Could some compromise be accomplished > by having the server read padding, but bound it by some upper limit > that should never be encountered by well behaved clients (maybe up to > 4K)? This is the reason for the code in SWUpdate, but the padding is limited to 512 bytes. I do not see any reason to increase it to 4K. > Beyond that it might actually be a DOS, but won't affect well > behaved clients. > > Also, this is not very good DOS protection; Best effort. > a bad client can just keep > re-opening the connection and sending more data on a new connection, We have to identify what SWUpdate provides and which is not SWUpdate's competence. If the SWU is sent by a custom client, it is duty of the client to ensure such as protection. For ways provided by SWUpdate, there are already implemented ways: if it works in pull mode, protection is done because libcurl will report an error with extremely slow connection (and retries, etc. can help if connection is really slow, such as GSM). In case of the embedded Webserver, the -t timeout parameter says SWUpdate that update is broken if no packets in "t" seconds are received. If connection is custom, the protection should be custom. > or send data really slowly and monopolize the connection. See above. > IMHO, the > current implementation is just hurting well behaved clients without > actually offering much protection. My comment related to the patch is to extend and make a not blocking read for the padding, if a wrong padding is added. My second comment is related to reset the offset variable, and I think this is wrong. > >> >> From SWUpdate point of view, it makes no sense to pad. The SWUGenerator >> project does not use cpio at all, and SWU are terminated with CPIO >> traile rwithout padding. It is thinkable to do the same in meta-swupdate. >> >>> First, stream_interface.c's extract_files() reset its offset variable >>> before processing each file. >> >> This is the way it is thought. offset is valid just for the extraction >> of each artifact and not valid anymore. >> >>> This value is 4-byte aligned >> >> This is specified in cpio >> >>> but not >>> 512-byte aligned, so when it was used by cpio-utils.c's >>> extract_padding() to compute the expected padding, it would produce an >>> incorrect padding amount. >> >> Reading the padding is a sort of best effort. >> >>> >>> Second, cpio-utils.c's extract_padding() could fail to read all padding >>> in cases where the client uses a smaller chunk size, >>> especially one less >>> than the padding size, such as the swupdate-client reference >>> implementation's 256 byte chunk size. >> >> I do not remmember why I set it 256 bytes, probably it was a copy&paste, >> it is surely too small, this can be changed. >> >>> >>> Signed-off-by: Nick Potenski <nick.potenski@garmin.com> >>> --- >>> core/cpio_utils.c | 14 +++++++++++--- >>> core/stream_interface.c | 1 - >>> 2 files changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/core/cpio_utils.c b/core/cpio_utils.c >>> index bcf4818..6408c8d 100644 >>> --- a/core/cpio_utils.c >>> +++ b/core/cpio_utils.c >>> @@ -106,14 +106,22 @@ void extract_padding(int fd, unsigned long *offset) >>> return; >>> >>> padding = (512 - (*offset % 512)) % 512; >>> - if (padding) { >>> - TRACE("Expecting %d padding bytes at end-of-file", padding); >>> + if (!padding) >>> + return; >>> + >>> + TRACE("Expecting %d padding bytes at end-of-file", padding); >>> + do { >>> len = read(fd, buf, padding); >> >> And probably instead of blocking, we should try in not blocking mode. >> Data are just dummies. >> >>> if (len < 0) { >>> DEBUG("Failure while reading padding %d: %s", fd, strerror(errno)); >>> return; >>> } >>> - } >>> + padding -= len; >>> + } while (len > 0); >>> + >>> + if (padding > 0) >>> + DEBUG("Expected %d more bytes of padding", padding); >>> + return; >> >> I tend to ignore at all, it is not defined that the SWU *must* have a >> padding. >> >>> >>> return; >>> } >>> diff --git a/core/stream_interface.c b/core/stream_interface.c >>> index 34435fa..8c7a047 100644 >>> --- a/core/stream_interface.c >>> +++ b/core/stream_interface.c >>> @@ -229,7 +229,6 @@ static int extract_files(int fd, struct swupdate_cfg *software) >>> (skip == SKIP_FILE ? "Not required: skipping" : "required")); >>> >>> fdout = -1; >>> - offset = 0; >> >> This does not seem correct to me. >> >>> >>> /* >>> * If images are not streamed directly into the target >> Regards, Stefano Babic
The issue here is not that a wrong padding size was added. It is that the wrong padding size is expected. I understand that you don't want the offset variable to be changed, but then we must have some other way to track the total offset into the file so as to calculate the padding size correctly. On Friday, June 30, 2023 at 6:36:51 AM UTC-5 Stefano Babic wrote: > Hi Joshua, Nick, > > On 26.06.23 16:38, Joshua Watt wrote: > > On Fri, Jun 23, 2023 at 5:26 AM Stefano Babic <sba...@denx.de> wrote: > >> > >> Hi Nick, > >> > >> On 06.06.23 15:17, 'Nick Potenski' via swupdate wrote: > >>> The stream interface would not read all data from the socket due to two > >>> separate issues. This could cause swupdate to close the socket fd > >>> before a client has written all data, which results in an EPIPE on the > >>> client. > >>> > >> > >> Let's start to explain how this is thought to work. SWUpdate has two > >> interfaces (ctrl and progress), and results about the update should be > >> retrieved via the progress instead of relying on the sending of the > >> data. Anyway, the issue here is generated by standard cpio , that adds > >> padding at the end of the file. > >> > >> Reason for SWUpdate to close the pipe is to avoid that an attacker just > >> appends dummy data at the end of a valid SWU creating a DOS attack if it > >> is not recognized by SWUpdate. That means SWUpdate will read some bytes > >> more (padding generated by cpio), and then it closes definetly > >> independently if more data will be sent. > > > > The problem with this approach is that it means that legitimate > > clients have no way of knowing if the server got the entire update or > > not; when the EPIPE fails the client is left wondering how much was > > actually sent and can't differentiate between the server simply > > decided to close the connection because it thinks padding is > > unimportant and the server disconnecting because e.g. crashed or > > rejected the connection, etc. > > In fact, decision should be done implementing the progress API, too. But > agree this behavior is nasty. > > > > > When dealing with sockets, it's really good practice for the server to > > read everything sent to it until it becomes apparent the connection is > > going nowhere and it terminates. > > Well, the SWU contains a termination (the TRAILER string), and SWUpdate > can detect if EOF is reached. Any data after the TRAILER is detected is > useless. > > > Could some compromise be accomplished > > by having the server read padding, but bound it by some upper limit > > that should never be encountered by well behaved clients (maybe up to > > 4K)? > > This is the reason for the code in SWUpdate, but the padding is limited > to 512 bytes. I do not see any reason to increase it to 4K. > > > Beyond that it might actually be a DOS, but won't affect well > > behaved clients. > > > > Also, this is not very good DOS protection; > > Best effort. > > > a bad client can just keep > > re-opening the connection and sending more data on a new connection, > > We have to identify what SWUpdate provides and which is not SWUpdate's > competence. If the SWU is sent by a custom client, it is duty of the > client to ensure such as protection. For ways provided by SWUpdate, > there are already implemented ways: if it works in pull mode, protection > is done because libcurl will report an error with extremely slow > connection (and retries, etc. can help if connection is really slow, > such as GSM). In case of the embedded Webserver, the -t timeout > parameter says SWUpdate that update is broken if no packets in "t" > seconds are received. If connection is custom, the protection should be > custom. > > > or send data really slowly and monopolize the connection. > > See above. > > > IMHO, the > > current implementation is just hurting well behaved clients without > > actually offering much protection. > > My comment related to the patch is to extend and make a not blocking > read for the padding, if a wrong padding is added. My second comment is > related to reset the offset variable, and I think this is wrong. > > > > >> > >> From SWUpdate point of view, it makes no sense to pad. The SWUGenerator > >> project does not use cpio at all, and SWU are terminated with CPIO > >> traile rwithout padding. It is thinkable to do the same in > meta-swupdate. > >> > >>> First, stream_interface.c's extract_files() reset its offset variable > >>> before processing each file. > >> > >> This is the way it is thought. offset is valid just for the extraction > >> of each artifact and not valid anymore. > >> > >>> This value is 4-byte aligned > >> > >> This is specified in cpio > >> > >>> but not > >>> 512-byte aligned, so when it was used by cpio-utils.c's > >>> extract_padding() to compute the expected padding, it would produce an > >>> incorrect padding amount. > >> > >> Reading the padding is a sort of best effort. > >> > >>> > >>> Second, cpio-utils.c's extract_padding() could fail to read all padding > >>> in cases where the client uses a smaller chunk size, > >>> especially one less > >>> than the padding size, such as the swupdate-client reference > >>> implementation's 256 byte chunk size. > >> > >> I do not remmember why I set it 256 bytes, probably it was a copy&paste, > >> it is surely too small, this can be changed. > >> > >>> > >>> Signed-off-by: Nick Potenski <nick.p...@garmin.com> > >>> --- > >>> core/cpio_utils.c | 14 +++++++++++--- > >>> core/stream_interface.c | 1 - > >>> 2 files changed, 11 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/core/cpio_utils.c b/core/cpio_utils.c > >>> index bcf4818..6408c8d 100644 > >>> --- a/core/cpio_utils.c > >>> +++ b/core/cpio_utils.c > >>> @@ -106,14 +106,22 @@ void extract_padding(int fd, unsigned long > *offset) > >>> return; > >>> > >>> padding = (512 - (*offset % 512)) % 512; > >>> - if (padding) { > >>> - TRACE("Expecting %d padding bytes at end-of-file", padding); > >>> + if (!padding) > >>> + return; > >>> + > >>> + TRACE("Expecting %d padding bytes at end-of-file", padding); > >>> + do { > >>> len = read(fd, buf, padding); > >> > >> And probably instead of blocking, we should try in not blocking mode. > >> Data are just dummies. > >> > >>> if (len < 0) { > >>> DEBUG("Failure while reading padding %d: %s", fd, strerror(errno)); > >>> return; > >>> } > >>> - } > >>> + padding -= len; > >>> + } while (len > 0); > >>> + > >>> + if (padding > 0) > >>> + DEBUG("Expected %d more bytes of padding", padding); > >>> + return; > >> > >> I tend to ignore at all, it is not defined that the SWU *must* have a > >> padding. > >> > >>> > >>> return; > >>> } > >>> diff --git a/core/stream_interface.c b/core/stream_interface.c > >>> index 34435fa..8c7a047 100644 > >>> --- a/core/stream_interface.c > >>> +++ b/core/stream_interface.c > >>> @@ -229,7 +229,6 @@ static int extract_files(int fd, struct > swupdate_cfg *software) > >>> (skip == SKIP_FILE ? "Not required: skipping" : "required")); > >>> > >>> fdout = -1; > >>> - offset = 0; > >> > >> This does not seem correct to me. > >> > >>> > >>> /* > >>> * If images are not streamed directly into the target > >> > > > Regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
On Fri, Jun 30, 2023 at 6:36 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi Joshua, Nick, > > On 26.06.23 16:38, Joshua Watt wrote: > > On Fri, Jun 23, 2023 at 5:26 AM Stefano Babic <sbabic@denx.de> wrote: > >> > >> Hi Nick, > >> > >> On 06.06.23 15:17, 'Nick Potenski' via swupdate wrote: > >>> The stream interface would not read all data from the socket due to two > >>> separate issues. This could cause swupdate to close the socket fd > >>> before a client has written all data, which results in an EPIPE on the > >>> client. > >>> > >> > >> Let's start to explain how this is thought to work. SWUpdate has two > >> interfaces (ctrl and progress), and results about the update should be > >> retrieved via the progress instead of relying on the sending of the > >> data. Anyway, the issue here is generated by standard cpio , that adds > >> padding at the end of the file. > >> > >> Reason for SWUpdate to close the pipe is to avoid that an attacker just > >> appends dummy data at the end of a valid SWU creating a DOS attack if it > >> is not recognized by SWUpdate. That means SWUpdate will read some bytes > >> more (padding generated by cpio), and then it closes definetly > >> independently if more data will be sent. > > > > The problem with this approach is that it means that legitimate > > clients have no way of knowing if the server got the entire update or > > not; when the EPIPE fails the client is left wondering how much was > > actually sent and can't differentiate between the server simply > > decided to close the connection because it thinks padding is > > unimportant and the server disconnecting because e.g. crashed or > > rejected the connection, etc. > > In fact, decision should be done implementing the progress API, too. But > agree this behavior is nasty. > > > > > When dealing with sockets, it's really good practice for the server to > > read everything sent to it until it becomes apparent the connection is > > going nowhere and it terminates. > > Well, the SWU contains a termination (the TRAILER string), and SWUpdate > can detect if EOF is reached. Any data after the TRAILER is detected is > useless. > > > Could some compromise be accomplished > > by having the server read padding, but bound it by some upper limit > > that should never be encountered by well behaved clients (maybe up to > > 4K)? > > This is the reason for the code in SWUpdate, but the padding is limited > to 512 bytes. I do not see any reason to increase it to 4K. > > > Beyond that it might actually be a DOS, but won't affect well > > behaved clients. > > > > Also, this is not very good DOS protection; > > Best effort. > > > a bad client can just keep > > re-opening the connection and sending more data on a new connection, > > We have to identify what SWUpdate provides and which is not SWUpdate's > competence. If the SWU is sent by a custom client, it is duty of the > client to ensure such as protection. For ways provided by SWUpdate, > there are already implemented ways: if it works in pull mode, protection > is done because libcurl will report an error with extremely slow > connection (and retries, etc. can help if connection is really slow, > such as GSM). In case of the embedded Webserver, the -t timeout > parameter says SWUpdate that update is broken if no packets in "t" > seconds are received. If connection is custom, the protection should be > custom. > > > or send data really slowly and monopolize the connection. > > See above. > > > IMHO, the > > current implementation is just hurting well behaved clients without > > actually offering much protection. > > My comment related to the patch is to extend and make a not blocking > read for the padding, if a wrong padding is added. My second comment is > related to reset the offset variable, and I think this is wrong. Fully non-blocking isn't quite ideal, because it's still a race if the data happens to be there when the server checks; perhaps it should be a read with a timeout? In that case, perhaps _all_ reads that the server does from the socket (not just for padding) should be a read with timeout? > > > > >> > >> From SWUpdate point of view, it makes no sense to pad. The SWUGenerator > >> project does not use cpio at all, and SWU are terminated with CPIO > >> traile rwithout padding. It is thinkable to do the same in meta-swupdate. > >> > >>> First, stream_interface.c's extract_files() reset its offset variable > >>> before processing each file. > >> > >> This is the way it is thought. offset is valid just for the extraction > >> of each artifact and not valid anymore. > >> > >>> This value is 4-byte aligned > >> > >> This is specified in cpio > >> > >>> but not > >>> 512-byte aligned, so when it was used by cpio-utils.c's > >>> extract_padding() to compute the expected padding, it would produce an > >>> incorrect padding amount. > >> > >> Reading the padding is a sort of best effort. > >> > >>> > >>> Second, cpio-utils.c's extract_padding() could fail to read all padding > >>> in cases where the client uses a smaller chunk size, > >>> especially one less > >>> than the padding size, such as the swupdate-client reference > >>> implementation's 256 byte chunk size. > >> > >> I do not remmember why I set it 256 bytes, probably it was a copy&paste, > >> it is surely too small, this can be changed. > >> > >>> > >>> Signed-off-by: Nick Potenski <nick.potenski@garmin.com> > >>> --- > >>> core/cpio_utils.c | 14 +++++++++++--- > >>> core/stream_interface.c | 1 - > >>> 2 files changed, 11 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/core/cpio_utils.c b/core/cpio_utils.c > >>> index bcf4818..6408c8d 100644 > >>> --- a/core/cpio_utils.c > >>> +++ b/core/cpio_utils.c > >>> @@ -106,14 +106,22 @@ void extract_padding(int fd, unsigned long *offset) > >>> return; > >>> > >>> padding = (512 - (*offset % 512)) % 512; > >>> - if (padding) { > >>> - TRACE("Expecting %d padding bytes at end-of-file", padding); > >>> + if (!padding) > >>> + return; > >>> + > >>> + TRACE("Expecting %d padding bytes at end-of-file", padding); > >>> + do { > >>> len = read(fd, buf, padding); > >> > >> And probably instead of blocking, we should try in not blocking mode. > >> Data are just dummies. > >> > >>> if (len < 0) { > >>> DEBUG("Failure while reading padding %d: %s", fd, strerror(errno)); > >>> return; > >>> } > >>> - } > >>> + padding -= len; > >>> + } while (len > 0); > >>> + > >>> + if (padding > 0) > >>> + DEBUG("Expected %d more bytes of padding", padding); > >>> + return; > >> > >> I tend to ignore at all, it is not defined that the SWU *must* have a > >> padding. > >> > >>> > >>> return; > >>> } > >>> diff --git a/core/stream_interface.c b/core/stream_interface.c > >>> index 34435fa..8c7a047 100644 > >>> --- a/core/stream_interface.c > >>> +++ b/core/stream_interface.c > >>> @@ -229,7 +229,6 @@ static int extract_files(int fd, struct swupdate_cfg *software) > >>> (skip == SKIP_FILE ? "Not required: skipping" : "required")); > >>> > >>> fdout = -1; > >>> - offset = 0; > >> > >> This does not seem correct to me. > >> > >>> > >>> /* > >>> * If images are not streamed directly into the target > >> > > > Regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > =====================================================================
Hi Nick, On 05.07.23 15:26, 'Nick Potenski' via swupdate wrote: > The issue here is not that a wrong padding size was added. It is that > the wrong padding size is expected. I understand that you don't want > the offset variable to be changed, but then we must have some other way > to track the total offset into the file so as to calculate the padding > size correctly. But is it needed to know *exactly* the padding size ? It is not data used by SWUpdate, it is just read and discarded. We can always read up to 512 bytes with a timeout (select + read). Best regards, Stefano Babic > > On Friday, June 30, 2023 at 6:36:51 AM UTC-5 Stefano Babic wrote: > > Hi Joshua, Nick, > > On 26.06.23 16:38, Joshua Watt wrote: > > On Fri, Jun 23, 2023 at 5:26 AM Stefano Babic <sba...@denx.de> > wrote: > >> > >> Hi Nick, > >> > >> On 06.06.23 15:17, 'Nick Potenski' via swupdate wrote: > >>> The stream interface would not read all data from the socket > due to two > >>> separate issues. This could cause swupdate to close the socket fd > >>> before a client has written all data, which results in an EPIPE > on the > >>> client. > >>> > >> > >> Let's start to explain how this is thought to work. SWUpdate has > two > >> interfaces (ctrl and progress), and results about the update > should be > >> retrieved via the progress instead of relying on the sending of the > >> data. Anyway, the issue here is generated by standard cpio , > that adds > >> padding at the end of the file. > >> > >> Reason for SWUpdate to close the pipe is to avoid that an > attacker just > >> appends dummy data at the end of a valid SWU creating a DOS > attack if it > >> is not recognized by SWUpdate. That means SWUpdate will read > some bytes > >> more (padding generated by cpio), and then it closes definetly > >> independently if more data will be sent. > > > > The problem with this approach is that it means that legitimate > > clients have no way of knowing if the server got the entire > update or > > not; when the EPIPE fails the client is left wondering how much was > > actually sent and can't differentiate between the server simply > > decided to close the connection because it thinks padding is > > unimportant and the server disconnecting because e.g. crashed or > > rejected the connection, etc. > > In fact, decision should be done implementing the progress API, too. > But > agree this behavior is nasty. > > > > > When dealing with sockets, it's really good practice for the > server to > > read everything sent to it until it becomes apparent the > connection is > > going nowhere and it terminates. > > Well, the SWU contains a termination (the TRAILER string), and SWUpdate > can detect if EOF is reached. Any data after the TRAILER is detected is > useless. > > > Could some compromise be accomplished > > by having the server read padding, but bound it by some upper limit > > that should never be encountered by well behaved clients (maybe > up to > > 4K)? > > This is the reason for the code in SWUpdate, but the padding is limited > to 512 bytes. I do not see any reason to increase it to 4K. > > > Beyond that it might actually be a DOS, but won't affect well > > behaved clients. > > > > Also, this is not very good DOS protection; > > Best effort. > > > a bad client can just keep > > re-opening the connection and sending more data on a new connection, > > We have to identify what SWUpdate provides and which is not SWUpdate's > competence. If the SWU is sent by a custom client, it is duty of the > client to ensure such as protection. For ways provided by SWUpdate, > there are already implemented ways: if it works in pull mode, > protection > is done because libcurl will report an error with extremely slow > connection (and retries, etc. can help if connection is really slow, > such as GSM). In case of the embedded Webserver, the -t timeout > parameter says SWUpdate that update is broken if no packets in "t" > seconds are received. If connection is custom, the protection should be > custom. > > > or send data really slowly and monopolize the connection. > > See above. > > > IMHO, the > > current implementation is just hurting well behaved clients without > > actually offering much protection. > > My comment related to the patch is to extend and make a not blocking > read for the padding, if a wrong padding is added. My second comment is > related to reset the offset variable, and I think this is wrong. > > > > >> > >> From SWUpdate point of view, it makes no sense to pad. The > SWUGenerator > >> project does not use cpio at all, and SWU are terminated with CPIO > >> traile rwithout padding. It is thinkable to do the same in > meta-swupdate. > >> > >>> First, stream_interface.c's extract_files() reset its offset > variable > >>> before processing each file. > >> > >> This is the way it is thought. offset is valid just for the > extraction > >> of each artifact and not valid anymore. > >> > >>> This value is 4-byte aligned > >> > >> This is specified in cpio > >> > >>> but not > >>> 512-byte aligned, so when it was used by cpio-utils.c's > >>> extract_padding() to compute the expected padding, it would > produce an > >>> incorrect padding amount. > >> > >> Reading the padding is a sort of best effort. > >> > >>> > >>> Second, cpio-utils.c's extract_padding() could fail to read all > padding > >>> in cases where the client uses a smaller chunk size, > >>> especially one less > >>> than the padding size, such as the swupdate-client reference > >>> implementation's 256 byte chunk size. > >> > >> I do not remmember why I set it 256 bytes, probably it was a > copy&paste, > >> it is surely too small, this can be changed. > >> > >>> > >>> Signed-off-by: Nick Potenski <nick.p...@garmin.com> > >>> --- > >>> core/cpio_utils.c | 14 +++++++++++--- > >>> core/stream_interface.c | 1 - > >>> 2 files changed, 11 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/core/cpio_utils.c b/core/cpio_utils.c > >>> index bcf4818..6408c8d 100644 > >>> --- a/core/cpio_utils.c > >>> +++ b/core/cpio_utils.c > >>> @@ -106,14 +106,22 @@ void extract_padding(int fd, unsigned > long *offset) > >>> return; > >>> > >>> padding = (512 - (*offset % 512)) % 512; > >>> - if (padding) { > >>> - TRACE("Expecting %d padding bytes at end-of-file", padding); > >>> + if (!padding) > >>> + return; > >>> + > >>> + TRACE("Expecting %d padding bytes at end-of-file", padding); > >>> + do { > >>> len = read(fd, buf, padding); > >> > >> And probably instead of blocking, we should try in not blocking > mode. > >> Data are just dummies. > >> > >>> if (len < 0) { > >>> DEBUG("Failure while reading padding %d: %s", fd, > strerror(errno)); > >>> return; > >>> } > >>> - } > >>> + padding -= len; > >>> + } while (len > 0); > >>> + > >>> + if (padding > 0) > >>> + DEBUG("Expected %d more bytes of padding", padding); > >>> + return; > >> > >> I tend to ignore at all, it is not defined that the SWU *must* > have a > >> padding. > >> > >>> > >>> return; > >>> } > >>> diff --git a/core/stream_interface.c b/core/stream_interface.c > >>> index 34435fa..8c7a047 100644 > >>> --- a/core/stream_interface.c > >>> +++ b/core/stream_interface.c > >>> @@ -229,7 +229,6 @@ static int extract_files(int fd, struct > swupdate_cfg *software) > >>> (skip == SKIP_FILE ? "Not required: skipping" : "required")); > >>> > >>> fdout = -1; > >>> - offset = 0; > >> > >> This does not seem correct to me. > >> > >>> > >>> /* > >>> * If images are not streamed directly into the target > >> > > > Regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: > +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/b76b7655-b095-425d-8478-2992ad262815n%40googlegroups.com <https://groups.google.com/d/msgid/swupdate/b76b7655-b095-425d-8478-2992ad262815n%40googlegroups.com?utm_medium=email&utm_source=footer>.
Hi Joshua, On 05.07.23 15:49, Joshua Watt wrote: > On Fri, Jun 30, 2023 at 6:36 AM Stefano Babic <sbabic@denx.de> wrote: >> >> Hi Joshua, Nick, >> >> On 26.06.23 16:38, Joshua Watt wrote: >>> On Fri, Jun 23, 2023 at 5:26 AM Stefano Babic <sbabic@denx.de> wrote: >>>> >>>> Hi Nick, >>>> >>>> On 06.06.23 15:17, 'Nick Potenski' via swupdate wrote: >>>>> The stream interface would not read all data from the socket due to two >>>>> separate issues. This could cause swupdate to close the socket fd >>>>> before a client has written all data, which results in an EPIPE on the >>>>> client. >>>>> >>>> >>>> Let's start to explain how this is thought to work. SWUpdate has two >>>> interfaces (ctrl and progress), and results about the update should be >>>> retrieved via the progress instead of relying on the sending of the >>>> data. Anyway, the issue here is generated by standard cpio , that adds >>>> padding at the end of the file. >>>> >>>> Reason for SWUpdate to close the pipe is to avoid that an attacker just >>>> appends dummy data at the end of a valid SWU creating a DOS attack if it >>>> is not recognized by SWUpdate. That means SWUpdate will read some bytes >>>> more (padding generated by cpio), and then it closes definetly >>>> independently if more data will be sent. >>> >>> The problem with this approach is that it means that legitimate >>> clients have no way of knowing if the server got the entire update or >>> not; when the EPIPE fails the client is left wondering how much was >>> actually sent and can't differentiate between the server simply >>> decided to close the connection because it thinks padding is >>> unimportant and the server disconnecting because e.g. crashed or >>> rejected the connection, etc. >> >> In fact, decision should be done implementing the progress API, too. But >> agree this behavior is nasty. >> >>> >>> When dealing with sockets, it's really good practice for the server to >>> read everything sent to it until it becomes apparent the connection is >>> going nowhere and it terminates. >> >> Well, the SWU contains a termination (the TRAILER string), and SWUpdate >> can detect if EOF is reached. Any data after the TRAILER is detected is >> useless. >> >>> Could some compromise be accomplished >>> by having the server read padding, but bound it by some upper limit >>> that should never be encountered by well behaved clients (maybe up to >>> 4K)? >> >> This is the reason for the code in SWUpdate, but the padding is limited >> to 512 bytes. I do not see any reason to increase it to 4K. >> >>> Beyond that it might actually be a DOS, but won't affect well >>> behaved clients. >>> >>> Also, this is not very good DOS protection; >> >> Best effort. >> >>> a bad client can just keep >>> re-opening the connection and sending more data on a new connection, >> >> We have to identify what SWUpdate provides and which is not SWUpdate's >> competence. If the SWU is sent by a custom client, it is duty of the >> client to ensure such as protection. For ways provided by SWUpdate, >> there are already implemented ways: if it works in pull mode, protection >> is done because libcurl will report an error with extremely slow >> connection (and retries, etc. can help if connection is really slow, >> such as GSM). In case of the embedded Webserver, the -t timeout >> parameter says SWUpdate that update is broken if no packets in "t" >> seconds are received. If connection is custom, the protection should be >> custom. >> >>> or send data really slowly and monopolize the connection. >> >> See above. >> >>> IMHO, the >>> current implementation is just hurting well behaved clients without >>> actually offering much protection. >> >> My comment related to the patch is to extend and make a not blocking >> read for the padding, if a wrong padding is added. My second comment is >> related to reset the offset variable, and I think this is wrong. > > Fully non-blocking isn't quite ideal, because it's still a race if the > data happens to be there when the server checks; perhaps it should be > a read with a timeout? Yes, I meant with a select() + timeout. > In that case, perhaps _all_ reads that the > server does from the socket (not just for padding) should be a read > with timeout? This is difficult because the workflow relies on blocking calls. Timeout are managed by the client of the socket, that is the wWebserver / client in suricatta, etc. Regards, Stefano Babic
diff --git a/core/cpio_utils.c b/core/cpio_utils.c index bcf4818..6408c8d 100644 --- a/core/cpio_utils.c +++ b/core/cpio_utils.c @@ -106,14 +106,22 @@ void extract_padding(int fd, unsigned long *offset) return; padding = (512 - (*offset % 512)) % 512; - if (padding) { - TRACE("Expecting %d padding bytes at end-of-file", padding); + if (!padding) + return; + + TRACE("Expecting %d padding bytes at end-of-file", padding); + do { len = read(fd, buf, padding); if (len < 0) { DEBUG("Failure while reading padding %d: %s", fd, strerror(errno)); return; } - } + padding -= len; + } while (len > 0); + + if (padding > 0) + DEBUG("Expected %d more bytes of padding", padding); + return; return; } diff --git a/core/stream_interface.c b/core/stream_interface.c index 34435fa..8c7a047 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -229,7 +229,6 @@ static int extract_files(int fd, struct swupdate_cfg *software) (skip == SKIP_FILE ? "Not required: skipping" : "required")); fdout = -1; - offset = 0; /* * If images are not streamed directly into the target
The stream interface would not read all data from the socket due to two separate issues. This could cause swupdate to close the socket fd before a client has written all data, which results in an EPIPE on the client. First, stream_interface.c's extract_files() reset its offset variable before processing each file. This value is 4-byte aligned but not 512-byte aligned, so when it was used by cpio-utils.c's extract_padding() to compute the expected padding, it would produce an incorrect padding amount. Second, cpio-utils.c's extract_padding() could fail to read all padding in cases where the client uses a smaller chunk size, especially one less than the padding size, such as the swupdate-client reference implementation's 256 byte chunk size. Signed-off-by: Nick Potenski <nick.potenski@garmin.com> --- core/cpio_utils.c | 14 +++++++++++--- core/stream_interface.c | 1 - 2 files changed, 11 insertions(+), 4 deletions(-)