Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make RandUtils.h use DRBG_get_bytes() #10465

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Make RandUtils.h use DRBG_get_bytes() #10465

merged 2 commits into from
Oct 14, 2021

Conversation

tcarmelveilleux
Copy link
Contributor

Problem

  • RandUtils.h's GetRandU* are using rand() which requires proper seeding to be useful and is not spec-compliant (since all random numbers in all protocols are supposed to be backed by DRBG).
  • Random number generation needs to funnel to the centralized well-defined DRBG API which is backed by a high-quality algorithm.

Change overview

  • Move RandUtils.* from lib/support to src/crypto to break
    dependency cycle w/ CHIPCryptoPAL.h
  • Make RandUtils functions use DRBG_get_bytes(), which
    avoids issues of srand() improperly initialized or
    dependencies on rand() in core SDK code. All random numbers
    in core protocols are supposed, by spec, to be generated using
    a crypto-based DRBG
  • Fix needless inclusions of RandUtils.h
  • Move all uses of GetRandU* to the new location/version
  • Fix minor usage of snprintf() for large hex strings in DnsSd
    to use BytesToHex().

Testing

  • Functionally not easy to test with new tests, since it's replacing a RNG with another RNG. Integration testing still passes. RNG audit passes.
  • Unit tests still pass
  • Cert tests YAML still pass

@tcarmelveilleux
Make RandUtils.h use DRBG_get_bytes()
ddf1aed 
- Move RandUtils.* from lib/support to src/crypto to break
  dependency cycle w/ CHIPCryptoPAL.h
- Make RandUtils functions use DRBG_get_bytes(), which
  avoids issues of `srand()` improperly initialized or
  dependencies on `rand()` in core SDK code. All random numbers
  in core protocols are supposed, by spec, to be generated using
  a crypto-based DRBG
- Fix needless inclusions of RandUtils.h
- Move all uses of `GetRandU*` to the new location/version
- Fix minor usage of snprintf() for large hex strings in DnsSd
  to use BytesToHex().

Testing done: unit tests still pass. Cert tests still pass.
@tcarmelveilleux tcarmelveilleux added crypto spec Mismatch between spec and implementation labels Oct 13, 2021
@restyled-io restyled-io bot mentioned this pull request Oct 13, 2021
Closed
@pullapprove pullapprove bot requested a review from woody-apple October 13, 2021 20:50
@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 2f0bae6

File Section File VM
chip-qpg6100-lighting-example.out .heap 0 8
chip-qpg6100-lighting-example.out .bss 0 -8
chip-qpg6100-lighting-example.out .text -80 -80
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,4535
.debug_line,0,592
.debug_abbrev,0,559
.debug_loc,0,354
.debug_str,0,140
[Unmapped],0,80
.strtab,0,21
.heap,8,0
.shstrtab,0,3
.bss,-8,0
.symtab,0,-64
.text,-80,-80

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 2f0bae6

File Section File VM
chip-shell.elf device_handles 8 8
chip-shell.elf text -56 -56
chip-lock.elf [LOAD #3 [RW]] 0 24
chip-lock.elf bss 0 -24
chip-lock.elf text -80 -80
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,4448
.debug_abbrev,0,548
.debug_line,0,500
.debug_loc,0,400
.debug_str,0,48
.strtab,0,28
device_handles,8,8
text,-56,-56
.symtab,0,-64

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,4463
.debug_abbrev,0,538
.debug_line,0,515
.debug_loc,0,372
.debug_str,0,48
[LOAD #3 [RW]],24,0
.strtab,0,21
.shstrtab,0,-1
bss,-24,0
.symtab,0,-64
text,-80,-80

@github-actions
Copy link

Size increase report for "esp32-example-build" from 2f0bae6

File Section File VM
chip-all-clusters-app.elf .flash.text 44 44
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,19266
.debug_line,0,1023
.debug_abbrev,0,811
.debug_loc,0,187
.debug_ranges,0,80
.flash.text,44,44
.strtab,0,44
.debug_str,0,22
.symtab,0,16
.riscv.attributes,0,3
.debug_frame,0,-12
[Unmapped],0,-44

@tcarmelveilleux tcarmelveilleux merged commit 267951c into project-chip:master Oct 14, 2021
@kghost kghost mentioned this pull request Oct 14, 2021
Closed
@github-actions
Copy link

github-actions bot commented Oct 14, 2021 •
edited
Loading

PR #10465: Size comparison from 2f0bae6 to 39e959b

12 builds
platform target config section 2f0bae6 39e959b change % change
esp32 all-clusters-app c3devkit .dram0.bss 60248 60248 0 0.0
.dram0.data 16232 16232 0 0.0
.flash.rodata 198088 198088 0 0.0
.flash.text 868770 868814 44 0.0
.iram0.text 57330 57330 0 0.0
m5stack .dram0.bss 62752 62752 0 0.0
.dram0.data 32084 32084 0 0.0
.flash.rodata 206676 206676 0 0.0
.flash.text 899899 899975 76 0.0
.iram0.text 125115 125115 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 bss 112312 112304 -8 -0.0
rodata 97160 97160 0 0.0
text 576936 576856 -80 -0.0
lock-app nrf52840dk_nrf52840 bss 111344 111336 -8 -0.0
rodata 93656 93656 0 0.0
text 558380 558300 -80 -0.0
pigweed-app nrf52840dk_nrf52840 bss 51772 51772 0 0.0
rodata 45772 45772 0 0.0
text 339392 339392 0 0.0
pump-app nrf52840dk_nrf52840 bss 111412 111404 -8 -0.0
rodata 94640 94640 0 0.0
text 561548 561468 -80 -0.0
pump-controller-app nrf52840dk_nrf52840 bss 111352 111344 -8 -0.0
rodata 93716 93716 0 0.0
text 558156 558076 -80 -0.0
shell nrf52840dk_nrf52840 bss 107328 107320 -8 -0.0
rodata 71336 71336 0 0.0
text 519020 518964 -56 -0.0
lighting-app nrf52840dk_nrf52840+rpc bss 108552 108544 -8 -0.0
rodata 87940 87940 0 0.0
text 550140 550060 -80 -0.0
nrf5340dk_nrf5340_cpuapp bss 113684 113676 -8 -0.0
rodata 92400 92400 0 0.0
text 506396 506316 -80 -0.0
lock-app nrf5340dk_nrf5340_cpuapp bss 112716 112708 -8 -0.0
rodata 88916 88916 0 0.0
text 487832 487752 -80 -0.0
shell nrf5340dk_nrf5340_cpuapp bss 108312 108304 -8 -0.0
rodata 65980 65980 0 0.0
text 439620 439564 -56 -0.0
14 builds
platform target config section 2f0bae6 39e959b change % change
efr32 lighting-app BRD4161A .bss 118012 118004 -8 -0.0
.data 1800 1800 0 0.0
.text 780980 780924 -56 -0.0
lock-app BRD4161A .bss 115884 115876 -8 -0.0
.data 1760 1760 0 0.0
.text 760236 760172 -64 -0.0
window-app BRD4161A .bss 116204 116196 -8 -0.0
.data 1764 1764 0 0.0
.text 761136 761088 -48 -0.0
lighting-app BRD4161A+rpc .bss 131340 131332 -8 -0.0
.data 1852 1852 0 0.0
.text 760728 760656 -72 -0.0
linux all-clusters-app debug .bss 52144 52144 0 0.0
.data 978 978 0 0.0
.data.rel.ro 58352 58352 0 0.0
.dynamic 592 592 0 0.0
.got 4080 4072 -8 -0.2
.init 27 27 0 0.0
.init_array 512 512 0 0.0
.rodata 134293 134581 288 0.2
.text 1316434 1316882 448 0.0
chip-tool debug .bss 17584 17584 0 0.0
.data 1584 1584 0 0.0
.data.rel.ro 77392 77392 0 0.0
.dynamic 592 592 0 0.0
.got 4328 4320 -8 -0.2
.init 27 27 0 0.0
.init_array 416 416 0 0.0
.rodata 173408 173664 256 0.1
.text 2424437 2424757 320 0.0
ota-provider-app debug .bss 37472 37472 0 0.0
.data 752 752 0 0.0
.data.rel.ro 23112 23112 0 0.0
.dynamic 592 592 0 0.0
.got 4016 4008 -8 -0.2
.init 27 27 0 0.0
.init_array 448 448 0 0.0
.rodata 108536 108792 256 0.2
.text 1004786 1005410 624 0.1
ota-requestor-app debug .bss 205728 205728 0 0.0
.data 752 752 0 0.0
.data.rel.ro 24408 24408 0 0.0
.dynamic 592 592 0 0.0
.got 4144 4136 -8 -0.2
.init 27 27 0 0.0
.init_array 520 520 0 0.0
.rodata 126576 126864 288 0.2
.text 1124594 1125042 448 0.0
shell debug .bss 16104 16104 0 0.0
.data 242 242 0 0.0
.data.rel.ro 35192 35184 -8 -0.0
.dynamic 592 592 0 0.0
.got 3512 3504 -8 -0.2
.init 27 27 0 0.0
.init_array 344 344 0 0.0
.rodata 70639 71311 672 1.0
.text 573842 574322 480 0.1
tv-app debug .bss 216368 216368 0 0.0
.data 2032 2032 0 0.0
.data.rel.ro 55536 55536 0 0.0
.dynamic 592 592 0 0.0
.got 4408 4400 -8 -0.2
.init 27 27 0 0.0
.init_array 616 616 0 0.0
.rodata 150248 150504 256 0.2
.text 1462946 1463394 448 0.0
bridge-app debug+rpc .bss 52880 52880 0 0.0
.data 976 976 0 0.0
.data.rel.ro 25480 25480 0 0.0
.dynamic 592 592 0 0.0
.got 3952 3944 -8 -0.2
.init 27 27 0 0.0
.init_array 408 408 0 0.0
.rodata 109212 109460 248 0.2
.text 1047045 1047493 448 0.0
lighting-app debug+rpc .bss 42200 42200 0 0.0
.data 1106 1106 0 0.0
.data.rel.ro 52192 52192 0 0.0
.dynamic 608 608 0 0.0
.got 4112 4104 -8 -0.2
.init 27 27 0 0.0
.init_array 536 536 0 0.0
.rodata 126353 126641 288 0.2
.text 1248194 1248642 448 0.0
mbed lighting-app CY8CPROTO_062_4343W+release .bss 172132 172132 0 0.0
.data 5464 5464 0 0.0
.heap 858848 858848 0 0.0
.text 1218656 1218656 0 0.0
lock-app CY8CPROTO_062_4343W+release .bss 171068 171068 0 0.0
.data 5432 5432 0 0.0
.heap 859944 859944 0 0.0
.text 1196648 1196648 0 0.0

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

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@woody-apple woody-apple woody-apple approved these changes

@msandstedt msandstedt msandstedt approved these changes

@andy31415 andy31415 andy31415 approved these changes

@mspang mspang mspang approved these changes

@anush-apple anush-apple Awaiting requested review from anush-apple

@balducci-apple balducci-apple Awaiting requested review from balducci-apple

@carol-apple carol-apple Awaiting requested review from carol-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chulspro chulspro Awaiting requested review from chulspro

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@franck-apple franck-apple Awaiting requested review from franck-apple

@hawk248 hawk248 Awaiting requested review from hawk248

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@LuDuda LuDuda Awaiting requested review from LuDuda

@pan-apple pan-apple Awaiting requested review from pan-apple

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@shana-apple shana-apple Awaiting requested review from shana-apple

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@wbschiller wbschiller Awaiting requested review from wbschiller

Assignees
No one assigned
Labels
crypto examples lib review - approved setup payload spec Mismatch between spec and implementation transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tcarmelveilleux @mspang @andy31415 @msandstedt @woody-apple @bzbarsky-apple @restyled-commits