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.

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

Fixed SQL injection vulnerability #2281

Merged
merged 1 commit into from Jun 13, 2018

Conversation

@grzesiek2010
Copy link
Member

commented Jun 8, 2018

What has been done to verify that this works as intended?

I tested query/update/delete in FormsProvider and InstanceProvider to check for regression.

Why is this the best possible solution? Were any other approaches considered?

It's requested by Google https://support.google.com/faqs/answer/7668308

a message we received in GPC says:


Vulnerable classes:

Lorg/odk/collect/android/provider/FormsProvider;->query
Lorg/odk/collect/android/provider/InstanceProvider;->query
Lorg/odk/collect/android/provider/InstanceProvider;->update

I don't know why they mentioned update only from InstanceProvider and not mentioned delete methods. The code in all cases looks the same when it comes to SQL so I fixed all cases.

Are there any risks to merging this code? If so, what are they?

I shouldn't be but anyway we need to test this change.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • run ./gradlew pmd checkstyle lint findbugs and confirmed all checks still pass.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 force-pushed the grzesiek2010:sqlInjection branch from e0dc448 to 99bc678 Jun 8, 2018

@grzesiek2010

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2018

Ok so for testing stuff: We need to test free edited methods query, delete, update
query is used for example when we open a list with forms/instances
delete is used when we remove forms/instance
update for forms it's used when a form is updated and for instances when a form is saved

@mmarciniak90

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

Tested with success!

Verified on Android: 4.1, 4.2, 4.4, 5.1, 6.0, 7.0

Verified cases:

  • filled new form
  • edited saved form
  • download newest version of a form
  • delete form
  • delete instance
  • light/dark theme

@opendatakit-bot label "behavior verified"
@opendatakit-bot unlabel "needs testing"

@lognaturel lognaturel merged commit b0a7eda into opendatakit:master Jun 13, 2018

2 checks passed

ci/circleci: build_debug Your tests passed on CircleCI!
Details
ci/circleci: test_unit Your tests passed on CircleCI!
Details
lognaturel added a commit that referenced this pull request Jun 13, 2018
xsteelej pushed a commit to xsteelej/collect that referenced this pull request Jun 14, 2018
John Steele
Merge branch 'master' into issue_#620_break_dependancy_on_httpclient
* master: (71 commits)
  Replaced png icons with vectors (opendatakit#2288)
  Dark theme not applied when user load settings from collect.settings file (opendatakit#2245)
  Removed unused ic_menu_forward.png icon (opendatakit#2289)
  Remove toast informing about spaces in underlying values (opendatakit#2285)
  Fixed SQL injection vulnerability (opendatakit#2281)
  Collect more database logs
  Fixes issues after rebasing onto master
  Add checkstyle rule to enforce CONSTANT_NAME pattern
  Remove boolean constant from ListMultiWidget Fix constant name
  Add final to make fields immutable
  Update AllWidgetsFormTest to match XLSForm in README (opendatakit#2256)
  Closes opendatakit#2269: Added null check for removedTempInstance() (opendatakit#2277)
  Replace the icons in DrawView with Vectors. (opendatakit#2282)
  Refactored HierarchyView using RecyclerView (opendatakit#2128)
  Issue opendatakit#2264 None translatable string-array time_scale in GeoTrace (opendatakit#2273)
  Replace the activity we show after clicking on a notification with a dialog (opendatakit#2236)
  Fixed NullPointerException in settings (opendatakit#2275)
  Render warning on question widget in red
  Add documentation for SpacesInUnderlyingValuesWarning
  Clean up: remove RelativeLayout leftover qualifier from LinearLayout
  ...

# Conflicts:
#	collect_app/src/main/java/org/odk/collect/android/tasks/InstanceServerUploader.java
#	collect_app/src/main/java/org/odk/collect/android/utilities/ResponseMessageParser.java
@@ -372,11 +382,22 @@ public int update(@NonNull Uri uri, ContentValues values, String where, String[]
}
}

String[] newWhereArgs;
if (whereArgs == null || whereArgs.length == 0) {

This comment has been minimized.

Copy link
@dcbriccetti

dcbriccetti Jun 17, 2018

Member

This duplicates code above.

dcbriccetti added a commit to dcbriccetti/collect that referenced this pull request Jun 17, 2018
xsteelej pushed a commit to xsteelej/collect that referenced this pull request Jun 25, 2018
John Steele
Merge branch 'master' into issue_#620_break_dependancy_on_httpclient
* master: (27 commits)
  Fixed NullPointerException (opendatakit#2317)
  Fixed NullPointerException (opendatakit#2318)
  Rank Widget (opendatakit#2180)
  Update JavaRosa (opendatakit#2314)
  Unify geoshape_google_layout and geoshape_osm_layout using fragments. (opendatakit#2305)
  Add pmd check for using isEmpty() for collections
  Add pmd check for writing asserts in a recommended manner (opendatakit#2312)
  Allocate id to the help text layout so that it may be later used to put answer layout below it
  Remove duplicate ids in the same layout
  Remove unused ids
  Update PMD (opendatakit#2306)
  Remove code duplication introduced in opendatakit#2281
  fixed typos
  Added UselessQualifiedThis to pmd ruleset to exclude the check so that GeoPointMapActivity.this can be used.
  Added a function to return an instance of the current class so that PMD passes due to it failing when .this is used incorrectly.
  Optimized usage imports for LocationClient
  converted several listeners to lambda expressions to simplify the activity a bit more.
  Added LocationListener to the MapReady function
  Style fixes
  Utilized the FileProvider for the Arbitrary File Widget
  ...

# Conflicts:
#	collect_app/src/main/java/org/odk/collect/android/receivers/NetworkReceiver.java
#	collect_app/src/main/java/org/odk/collect/android/utilities/FileUtils.java
@lognaturel lognaturel referenced this pull request Jul 17, 2018
2 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.