New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorporate upstream changes to libxls to address security vulnerabilities #441

Closed
jennybc opened this Issue Apr 12, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@jennybc
Member

jennybc commented Apr 12, 2018

Step 1: determine if said upstream changes actually exist and, if so, where:

evanmiller/libxls#1

This was already on the radar, but not yet in a form that gives me a clear path forward:

#409
#358 (comment)

@jennybc

This comment has been minimized.

Member

jennybc commented Apr 12, 2018


CVE-2017-12110: libxls xls_appendSST Code Execution Vulnerability
libxls 1.4 readxl package 1.0.0 for R (tested using Microsoft R 4.3.1)
evanmiller/libxls#2
https://www.talosintelligence.com/vulnerability_reports/TALOS-2017-0462
shows a crash via readxl and via xls2csv
First fixed 2017-11 in evanmiller/libxls@9a88843


CVE-2017-2896: libxls xls_mergedCells Code Execution Vulnerability
libxls 1.4 readxl package 1.0.0 for R (tested using Microsoft R 4.3.1)
evanmiller/libxls#6
https://www.talosintelligence.com/vulnerability_reports/TALOS-2017-0403
shows a crash via readxl and via xls2csv
First fixed 2017-10 in evanmiller/libxls@61a2274


CVE-2017-2897: libxls read_MSAT Code Execution Vulnerability
libxls 1.4 readxl package 1.0.0 for R (tested using Microsoft R 4.3.1)
evanmiller/libxls#5
https://www.talosintelligence.com/vulnerability_reports/TALOS-2017-0404
shows a crash via readxl and via xls2csv
First fixed 2017-10 in evanmiller/libxls@61a2274#diff-0e7a5461f817411e673c0608bc4bfdb5 (part of same commit that addressed previous CVE ^^)


CVE-2017-12111: libxls xls_addCell Formula Code Execution Vulnerability
libxls 1.4 <-- "However this particular vulnerability does not impact the readxl package."
evanmiller/libxls#3
https://www.talosintelligence.com/vulnerability_reports/TALOS-2017-0463
shows crash output only for libxls
First fixed 2014-04 in evanmiller/libxls@29c8c97


CVE-2017-2919: libxls xls_getfcell Code Execution Vulnerability
libxls 1.3.4 <-- note does not affect readxl, presumably because has always embedded a later version
evanmiller/libxls#4
https://www.talosintelligence.com/vulnerability_reports/TALOS-2017-0426
shows a crash only via xls2csv
First fixed 2012-03 in evanmiller/libxls@8189d02

@eddelbuettel

This comment has been minimized.

eddelbuettel commented Apr 13, 2018

I incorporated all of these these in this commit again Debian's readxl 1.0.0. Let me know if you want them as a diff, or file set, or ...

There may be a delta against what you have here if you too departed already from 1.0.0.

@jennybc

This comment has been minimized.

Member

jennybc commented Apr 13, 2018

The PR #442 is in a functional state now but still need to clear NOTEs.

@eddelbuettel

This comment has been minimized.

eddelbuettel commented Apr 13, 2018

Nice. It may still need some fine-tuning. Using my Debian-patched side of the code (which also consists of taking the patches by Evan)

R> library(readxl)
R> read_excel("mtcars.xls")
# A tibble: 6 x 10
    mpg   cyl  disp    hp  drat    wt  qsec    vs    am  gear
  <dbl> <dbl> <dbl> <dbl> <dbl> <dbl> <dbl> <dbl> <dbl> <dbl>
1  21.0    6.  160.  110.  3.90  2.62  16.5    0.    1.    4.
2  21.0    6.  160.  110.  3.90  2.88  17.0    0.    1.    4.
3  22.8    4.  108.   93.  3.85  2.32  18.6    1.    1.    4.
4  21.4    6.  258.  110.  3.08  3.22  19.4    1.    0.    3.
5  18.7    8.  360.  175.  3.15  3.44  17.0    0.    0.    3.
6  18.1    6.  225.  105.  2.76  3.46  20.2    1.    0.    3.
R> read_excel("dates-1900.xls")
Error in read_fun(path = path, sheet = sheet, limits = limits, shim = shim,  : 
  Failed to open dates-1900.xls
R> read_excel("dates-1904.xls")
Error in read_fun(path = path, sheet = sheet, limits = limits, shim = shim,  : 
  Failed to open dates-1904.xls
R> 

Or do you have that squatted away thanks to the other changes in the repo?

@jennybc

This comment has been minimized.

Member

jennybc commented Apr 13, 2018

Hmmm. I don't see this problem locally when I'm at HEAD of #442. I can read those files fine. They are also used in the tests, so they are being successfully read on Travis and AppVeyor.

devtools::load_all(".")
#> Loading readxl
read_excel("tests/testthat/sheets/dates-1900.xls", col_names = paste0("X", 1:5))
#> # A tibble: 1 x 5
#>   X1                  X2                  X3                 
#>   <dttm>              <dttm>              <dttm>             
#> 1 2000-01-01 00:00:00 2000-01-01 00:00:00 2000-01-01 00:00:00
#> # ... with 2 more variables: X4 <dttm>, X5 <dttm>
read_excel("tests/testthat/sheets/dates-1904.xls", col_names = paste0("X", 1:5))
#> # A tibble: 1 x 5
#>   X1                  X2                  X3                 
#>   <dttm>              <dttm>              <dttm>             
#> 1 2000-01-01 00:00:00 2000-01-01 00:00:00 2000-01-01 00:00:00
#> # ... with 2 more variables: X4 <dttm>, X5 <dttm>

Created on 2018-04-13 by the reprex package (v0.2.0).

@eddelbuettel

This comment has been minimized.

eddelbuettel commented Apr 13, 2018

Good to know. I made changes to the vanilla readxl 1.0.0 release. So call this a vote in favour of getting a new version out onto CRAN :)

@jennybc jennybc closed this in #442 Apr 14, 2018

@eddelbuettel

This comment has been minimized.

eddelbuettel commented Apr 14, 2018

When do you plan to submit the updated version to CRAN?

@jennybc

This comment has been minimized.

Member

jennybc commented Apr 14, 2018

I'm planning to submit early this week. Am currently pondering if there's any other low-hanging fruit to knock off quickly and also need to run revdepchecks.

@jennybc

This comment has been minimized.

Member

jennybc commented Apr 20, 2018

@eddelbuettel The new readxl -- v1.1.0 -- is on CRAN now.

@KyleHaynes

This comment has been minimized.

KyleHaynes commented Apr 20, 2018

@jennybc Thanks Jenny!

@eddelbuettel

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment