CellularPrivacy / Android-IMSI-Catcher-Detector

AIMSICD • Fight IMSI-Catcher, StingRay and silent SMS!
https://cellularprivacy.github.io/Android-IMSI-Catcher-Detector/
GNU General Public License v3.0
4.71k stars 944 forks source link

Using wrong type of SQL queries #549

Closed E3V3A closed 9 years ago

E3V3A commented 9 years ago

As I have stated over and again, and even clearly in the same file, this is exactly the wrong kind of queries we should use.

        return mDb.query( DBTableColumnIds.DBI_BTS_TABLE_NAME,
                new String[]{"Mcc", "Mnc", "Lac", "CellID", "Lng", "Lat", "Signal", "Timestamp",
                        "Accuracy", "Speed", "Direction", "NetworkType"}, "OCID_SUBMITTED <> 1",//<-- where we getting these tables OCID_SUBMITTED DBi_bts?/DBi_measure?/DBe_import?
                null, null, null, null
        );

**\ impossible to read!

Instead use rawQuery() like this:

        String query = String.format("SELECT * FROM %s WHERE %s = %d",
                DBTableColumnIds.DBI_BTS_TABLE_NAME,            //DBi_bts
                DBTableColumnIds.DBI_BTS_CID,  cell.getCID());  //CID
        Cursor bts_cursor = mDb.rawQuery(query,null);

Or even better, just write out the damn constants (table and column names) in clear text:

        String query = String.format(
                "SELECT * FROM DBi_bts WHERE CID = %d", 
                cell.getCID()
        );
        Cursor bts_cursor = mDb.rawQuery(query,null);

Please change all queries and Cursors to this.

ghost commented 9 years ago

@E3V3A its double the work to type these in hard coded form like this and not good for changes.

let me give you an example, I had to change lat in DBi_bts to to gps_lat because you didnt like how it looked, now I only had to change this value in the constants class and it was set everywhere this was used if I had to go with the hard coded method I would have to trace every possible place that this was used its prone to error's and triple the work. Personally from my programmers point of view I like this way and it speeds up development. If its that much of a problem its easy to show the full query in a comment.

This wasn't my query and was original code I just changed the table name

return mDb.query( DBTableColumnIds.DBI_BTS_TABLE_NAME, new String[]{"Mcc", "Mnc", "Lac", "CellID", "Lng", "Lat", "Signal", "Timestamp", "Accuracy", "Speed", "Direction", "NetworkType"}, "OCID_SUBMITTED <> 1",//<-- where we getting these tables OCID_SUBMITTED DBi_bts?/DBi_measure?/DBe_import? null, null, null, null );
E3V3A commented 9 years ago

No it's not, because you have to remember the names of all the new constants, and if you have the schema diagram here in front of your face, it's piece of cake to read. Besides all the constants have different names so their re-use is minimal. Not to mention the huge bloating effect of using those long CAPITAL constants. What's the purpose of moving the constants out from the file anyway? As far as I can see, none at all, on the contrary making the underlying SQL code really hard, if not impossible to read.

Right now I'm not able to neither check nor fix the huge number of comments you made in the DbAdapter.

You also didn't seem to have added or addressed the comments I made in the commit in https://github.com/E3V3A/woork/commit/404e67a78eff5d0d48e7df6759ccb65f28544428 ?

There's no point for me to edit and/or make any future comments here at all, if it's not taken up or keep getting removed or even rewritten everywhere. I'm going nuts here, thinking "Oh, haven't I already said or commented on this before?" and sure enough, going back and check, I have, only that it was removed in some poorly documented PR somewhere. (Hey, please note, I'm not saying you did this, but it has happened too much recently from others...)

As it is now, the DbAdapter is completely disorganized, there's little telling of what is created where and used where, and queried where, as it seem all mixed up. So it's pretty close to impossible to cross reference with the diagram...

E3V3A commented 9 years ago

PS. You changed the table name to use DBi_bts, but none of those columns exists in there, or did you change the internal DB as well??

(Not using the SQL script to create the aimsicd.db is making this impossible to sort out, without spending considerable time to go grab it manually and then perform all the required queries on it.)

E3V3A commented 9 years ago

I really hate to have to deal with these tables like this, becuse it will prevent us from creating more complex queries as required for other detections later on. Or how to debug DB views and indexes etc...

E3V3A commented 9 years ago

@banjaxbanjo I'm not saying I'm not able to be convinced otherwise, but so far there is nothing very convincing in your above arguments...that would outweigh the transparency offered as I suggested.

ghost commented 9 years ago

@E3V3A I put a todo on that query saying its not working and missing columns so I didnt touch the rest of it and I dont think it's called anywhere plus its not my code.(I left there because I didnt know what you where trying to achieve with it)

I agree it is pretty hard to read and understand the query from a non programming point of view but development wise this speeds up the dev. I commented the table and columns names as much as I could to show original table/column names.

I used your sql to create original database eva its exaclty as we agreed with all the right column names and tables.

I thought we agreed on this method eva before the pr and you said ok so its kinda frustrating now you are going back on it.

ghost commented 9 years ago

I've no problem changing tables to hard coded strings but its going to take time for me to do this, I dont know maybe its just me but the string format is cleaner instead of using "bbbb"+"cccc"+"1211". Will work on it when I'm a bit fresher

E3V3A commented 9 years ago

left there because I didnt know what you where trying to achieve with it

I didn't make it either, but it was used in deciding what cells to upload to OCID, by avoiding those that had already been uploaded.

query from a non programming point of view but development wise this speeds up the dev.

No, because you're the only one able to develop on this right now. Having transparency will allow others, including me, as I have before. Besides it is the SQL I'm talking about, not the Java extensions. As for using whatever you need to fill the tables, that is fine with me, but the queries themselves has to remain ultra-clear-crisp-concise, as close to raw SQL as possible. In addition, it is clear that most Android devs have little to no clue to using raw SQL, which make our off the charts DB very complex for them, thus requiring extensive documentation and clarity.

I used your sql to create original database eva its exaclty as we agreed with all the right column names and tables.

Just wasn't sure, since I didn't see a sql loader anywhere. I guess I've overlooked it.

I thought we agreed on this method eva before the pr and you said ok so its kinda frustrating now you are going back on it.

Yes, we did, but we're allowed to change our minds for the sake of better options. I know, and I'm sorry for that, but we don't have to change much. As I see it, most is working, but the bloated queries need some polishing. However, I'm not going to sit down and go through thousands of lines of code for commenting and studying unless:

E3V3A commented 9 years ago

I'm fine with using the String.format()...

ghost commented 9 years ago

@E3V3A well as long as I didn't do a complete fubar on this I'll happily sort the issue for you guys, my db style isnt for everyone but this is the way I leaned it from lynda.com videos on android development and my personal choice but I'll change for the masses

ghost commented 9 years ago

@E3V3A about these tables and indexes not being right can you point these out so we can sort it.

E3V3A commented 9 years ago

I have in various issues. Most notably you can see it by yourself in the DB Viewer where certain items have been switched around and others have nothing or ~0xfffffff in them. See #234 .

ghost commented 9 years ago

Ive only put in what was originally sent to these tables eva and half the values we can't get yet.

E3V3A commented 9 years ago

Ive only put in what was originally sent to these tables

I know, but it is not. Also some was wrong also before, I wrote some to you in email I think.

Now:

ghost commented 9 years ago

2015-07-15 00 55 28

2015-07-15 00 47 50 Hmm this is odd because mcc on my phone are dec not hex. Also rx signal is not correct I just used rssi for this because I wasn't sure what was what and this is giving me a weird value as you can see in image and I also get this with psc. I'm getting these values from the original code so this issue was here before this pr for me.

Am not 100% in what OCID tables are like what goes where this is where you need to switch for me.

I had to put timestamp in this format for SignalStrengthTracker because it was sending them like this in original code so the old code had problems when used in new code. Timestamp are System.Millis ()

E3V3A commented 9 years ago

please use the latest build with my changes, and screen shot showing your network connection stat

Yes, it's all in decimal...

ghost commented 9 years ago

You mean the 32 build on fdroid? Or

ghost commented 9 years ago

Ah wrong button

ghost commented 9 years ago

@E3V3A this PR was about db I'm not trying to redesign the whole gui here this is the way it previously was displayed

E3V3A commented 9 years ago

I know, I don't expect you to fix all that, but some of the issues above, are new. Especially the Measured Signal Strengths was working before. As was the GPS coordinate truncation, AFAICR. And AvgSignal and Range was not switched around.

ghost commented 9 years ago

@E3V3A I'm sure I've ask you to sort AvgSignal for me as I'd no idea what goes where as you can see in my comments with the pr

https://github.com/SecUpwN/Android-IMSI-Catcher-Detector/blob/development/app/src/main/java/com/SecUpwN/AIMSICD/adapters/AIMSICDDbAdapter.java#L1873-L1876

https://github.com/SecUpwN/Android-IMSI-Catcher-Detector/blob/development/app/src/main/java/com/SecUpwN/AIMSICD/adapters/AIMSICDDbAdapter.java#L1911-L1915

E3V3A commented 9 years ago

Ahh,

TX = Transmit, RX = Receive

So rx_power is almost the same as API signal strength (depending on RAT, but nver mind that), so use that. tx_power, we don't have, except in ServiceMode items, so you can comment it out.